Re: A cloned tap device patch (for a person to test KQEMU 1.4.0pre1 for QEMU 0.10.1)

[Sepherosa Ziehau <sepherosa@xxxxxxxxx>]

On Wed, May 6, 2009 at 8:31 AM, Naoya Sugioka <naoya.sugioka@gmail.com> wrote:
> Hello,
>
> As some of you already know, I wrote a kqemu patch for DragonFly, but
> I found very simple problem to use it.
> I cannot create tap0 cloned interface by ifconfig comamnd now, so I
> made a patch.
>
> Attachment is a suggested patch to create tap cloned tap device (only
> tap device) only applicapable with HEAD.
> I 'd like to post this patch to share with community and get a feedback from.

Here are some comments about the patch:

@@ -131,55 +151,28 @@ DEV_MODULE(if_tap, tapmodevent, NULL);
 static int
 tapmodevent(module_t mod, int type, void *data)
 {
-	static int		 attached = 0;
-	struct ifnet		*ifp = NULL;
-	int			 unit;
-
+	struct tap_softc*	tp;
 	switch (type) {
 	case MOD_LOAD:
-		if (attached)
+		if (!SLIST_EMPTY(&tap_listhead))
 			return (EEXIST);

tap_listhead is not initialized using static SLIST_HEAD_INITIALIZER,
so SLIST_EMPTY() here is not good (though tap_listhead is in bss and
is default to NULL, if it is ever changed to TAILQ, things are
broken).  And even if tap_listhead is empty, it does not mean the
module is not loaded :).  I didn't take a look at the code whether
module loading code has done the check (as it should do), but the
removed code is correct way, if module loading code does not do the
double loading check.


You will also need to insert the tap created in tapcreate() into the
local tap list, else upon module unloading you are leaking memory.  I
think you should factor code out of tapcreate() and tcp_clone_create()
to create tap, the refactored code could reduce code duplication and
avoid the error I mentioned.


Except for the indentation style (we use tab instead of 8 white space
:), rest of the patch looks good to me.

Best Regards,
sephe

-- 
Live Free or Die