Bug #1357

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

Added by naoya.sugioka over 5 years ago. Updated over 5 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

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.

I'm start using this to use QEMU/KQEMU as a virtual server of
DragonFly/FreeBSD. Unfortuantelly, I saw
DMA address error when using wirh Linux guest though...

thank you,
-Naoya

tap7.diff Magnifier (7.58 KB) naoya.sugioka, 05/06/2009 12:38 AM

History

#1 Updated by sepherosa over 5 years ago

On Wed, May 6, 2009 at 8:31 AM, Naoya Sugioka <> 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

#2 Updated by sepherosa over 5 years ago

On Sun, May 17, 2009 at 12:06 PM, Sepherosa Ziehau <> wrote:
> On Thu, May 14, 2009 at 1:11 PM, Naoya Sugioka <> wrote:
>> Hi Sephe,
>>
>> Thank you for your comment. Here I made a patch which includes you pointed.
>> I hope you will review this patch too.
>> - Naoya
>
> Sorry for the delay, I will review it today.

Could you test following three patches:
http://leaf.dragonflybsd.org/~sephe/0001-tap-4-Add-ifclone-support.patch
http://leaf.dragonflybsd.org/~sephe/0002-tap-4-Correct-reversed-logic.patch
http://leaf.dragonflybsd.org/~sephe/0003-tap-4-More-fixes-to-tapuponopen.patch

The first one is an adjusted version of your submitted patch.

Best Regards,
sephe

#3 Updated by sepherosa over 5 years ago

committed. Thank you for submission and testing!

Also available in: Atom PDF