[ANNOUNCE] kvm-83 release
diff mbox

Message ID 1231867653.4290.196.camel@localhost.localdomain
State Accepted, archived
Headers show

Commit Message

Mark McLoughlin Jan. 13, 2009, 5:27 p.m. UTC
Hi John,

On Wed, 2009-01-14 at 00:15 +0800, John Wong wrote:
> Hi, i installed kvm-83 on xubuntu-amd64/kernel-2.6.7-11.
> i notice some problem with -net downscript.
> my kvm start up script like this:
> qemu-system-x86_64 ... ... -net 
> tap,ifname=tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown
> when the guest shutdown, kvm pass all argument to downscript.
> 
> the command like this (kvm-83):
> /path/kvm-ifdown
> "tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown"
> 
> kvm-82 only pass tap0 to downscript like this:
> /path/kvm-ifdown "tap0"

Thanks for the report. Can you try this simple change and see if it
fixes it for you?


Thanks,
Mark.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

John Wong Jan. 13, 2009, 5:40 p.m. UTC | #1
Hi Mark, i want to try, but i don't know where can download snapshot
source code,
i'm long time no use linux.
Where can i download kvm snapshot source code? kernel.org? or?

On 2009年01月14日 01:27, Mark McLoughlin wrote:
> Hi John,
>
> On Wed, 2009-01-14 at 00:15 +0800, John Wong wrote:
>   
>> Hi, i installed kvm-83 on xubuntu-amd64/kernel-2.6.7-11.
>> i notice some problem with -net downscript.
>> my kvm start up script like this:
>> qemu-system-x86_64 ... ... -net 
>> tap,ifname=tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown
>> when the guest shutdown, kvm pass all argument to downscript.
>>
>> the command like this (kvm-83):
>> /path/kvm-ifdown
>> "tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown"
>>
>> kvm-82 only pass tap0 to downscript like this:
>> /path/kvm-ifdown "tap0"
>>     
>
> Thanks for the report. Can you try this simple change and see if it
> fixes it for you?
>
> --- qemu/net.c
> +++ qemu/net.c
> @@ -1740,7 +1740,7 @@ void net_cleanup(void)
>                  TAPState *s = vc->opaque;
>  
>                  if (strcmp(vc->model, "tap") == 0 &&
> -                    sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
> +                    sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
>                      s->down_script[0])
>                      launch_script(s->down_script, ifname, s->fd);
>              }
>
> Thanks,
> Mark.
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Shah Jan. 13, 2009, 6:22 p.m. UTC | #2
On Wed, Jan 14, 2009 at 01:40:27AM +0800, John Wong wrote:
> Hi Mark, i want to try, but i don't know where can download snapshot
> source code,
> i'm long time no use linux.
> Where can i download kvm snapshot source code? kernel.org? or?

Hi John,

You apply this patch on top of the kvm-83 release that you've already
downloaded. You don't need anything else.

Amit.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Tokarev Jan. 13, 2009, 8:32 p.m. UTC | #3
Mark McLoughlin wrote:
> Hi John,
> 
> On Wed, 2009-01-14 at 00:15 +0800, John Wong wrote:
>> Hi, i installed kvm-83 on xubuntu-amd64/kernel-2.6.7-11.
>> i notice some problem with -net downscript.
>> my kvm start up script like this:
>> qemu-system-x86_64 ... ... -net 
>> tap,ifname=tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown
>> when the guest shutdown, kvm pass all argument to downscript.
>>
>> the command like this (kvm-83):
>> /path/kvm-ifdown
>> "tap0,script=/path/kvm-ifup,downscript=/path/kvm-ifdown"
>>
>> kvm-82 only pass tap0 to downscript like this:
>> /path/kvm-ifdown "tap0"
> 
> Thanks for the report. Can you try this simple change and see if it
> fixes it for you?
> 
> --- qemu/net.c
> +++ qemu/net.c
> @@ -1740,7 +1740,7 @@ void net_cleanup(void)
>                  TAPState *s = vc->opaque;
>  
>                  if (strcmp(vc->model, "tap") == 0 &&
> -                    sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
> +                    sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&

Hmm.  What if ifname=foo is the last parameter?

/mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Tokarev Jan. 13, 2009, 8:35 p.m. UTC | #4
Michael Tokarev wrote:
> Mark McLoughlin wrote:
[]
>> --- qemu/net.c
>> +++ qemu/net.c
>> @@ -1740,7 +1740,7 @@ void net_cleanup(void)
>>                  TAPState *s = vc->opaque;
>>  
>>                  if (strcmp(vc->model, "tap") == 0 &&
>> -                    sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
>> +                    sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
> 
> Hmm.  What if ifname=foo is the last parameter?

And while we're at it.. Why the parsing isn't done like this:

  for(tok = strtok(arg, ","); tok; tok = strtok(NULL, ",")) {
    if (strncmp(tok, "ifname=", 7) == 0)
      strcpy(ifname, tok+7);
    else if ...
  }

-- here and everywhere else?

/mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Anthony Liguori Jan. 13, 2009, 8:37 p.m. UTC | #5
Michael Tokarev wrote:
> Michael Tokarev wrote:
>   
>> Mark McLoughlin wrote:
>>     
> []
>   
>>> --- qemu/net.c
>>> +++ qemu/net.c
>>> @@ -1740,7 +1740,7 @@ void net_cleanup(void)
>>>                  TAPState *s = vc->opaque;
>>>  
>>>                  if (strcmp(vc->model, "tap") == 0 &&
>>> -                    sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
>>> +                    sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
>>>       
>> Hmm.  What if ifname=foo is the last parameter?
>>     
>
> And while we're at it.. Why the parsing isn't done like this:
>
>   for(tok = strtok(arg, ","); tok; tok = strtok(NULL, ",")) {
>     if (strncmp(tok, "ifname=", 7) == 0)
>       strcpy(ifname, tok+7);
>     else if ...
>   }
>   

Because strtok is extremely evil.

Regards,

Anthony Liguori

> -- here and everywhere else?
>
> /mjt
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Tokarev Jan. 13, 2009, 9:01 p.m. UTC | #6
Anthony Liguori wrote:
>> Michael Tokarev wrote:
...
>>>>        if (strcmp(vc->model, "tap") == 0 &&
>>>> -          sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
>>>> +          sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
>>
>> And while we're at it.. Why the parsing isn't done like this:
>>
>>   for(tok = strtok(arg, ","); tok; tok = strtok(NULL, ",")) {
>>     if (strncmp(tok, "ifname=", 7) == 0)
>>       strcpy(ifname, tok+7);
>>     else if ...
>>   }
> 
> Because strtok is extremely evil.

I didn't mean strtok, but the way -- something LIKE that, i.e,
by splitting the string into args and parsing each in turn.
There's getsubopt(3) too, which does exactly that, and which
is used by, say, mount(8) to parse mount options (-o foo=bar
things).  Those home-grown sscanfs tend to become buggy like
in this example...

And if you dislike strtok especially, there are other alternatives.
And speaking of strtok, there's nothing evil in it provided you
don't run another strtok sequence while this one is running
(it's not reenterant, so to say).  Here, let it modify the
existing string and replace delimiters with zeros to terminate
our tokens, -- nothing wrong with that.  It's not worse than
silently truncating ifname size to 63 chars first and to
IF_NAMESIZE second.  If you don't want to modify original
string, strdup() or astrdup() are our friends...  But that's
all details, details, not related to the original issue and
question.

/mjt
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark McLoughlin Jan. 13, 2009, 10:30 p.m. UTC | #7
On Wed, 2009-01-14 at 00:01 +0300, Michael Tokarev wrote:
> Anthony Liguori wrote:
> >> Michael Tokarev wrote:
> ...
> >>>>        if (strcmp(vc->model, "tap") == 0 &&
> >>>> -          sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
> >>>> +          sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
> >>
> >> And while we're at it.. Why the parsing isn't done like this:
> >>
> >>   for(tok = strtok(arg, ","); tok; tok = strtok(NULL, ",")) {
> >>     if (strncmp(tok, "ifname=", 7) == 0)
> >>       strcpy(ifname, tok+7);
> >>     else if ...
> >>   }
> > 
> > Because strtok is extremely evil.
> 
> I didn't mean strtok, but the way -- something LIKE that, i.e,
> by splitting the string into args and parsing each in turn.

It shouldn't be parsing anything at all, it's a stupid lame hack.

I fixed it with:

  http://git.kernel.org/?p=virt/qemu/qemu.git;a=commit;h=973cbd37ce

Cheers,
Mark.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

--- qemu/net.c
+++ qemu/net.c
@@ -1740,7 +1740,7 @@  void net_cleanup(void)
                 TAPState *s = vc->opaque;
 
                 if (strcmp(vc->model, "tap") == 0 &&
-                    sscanf(vc->info_str, "ifname=%63s ", ifname) == 1 &&
+                    sscanf(vc->info_str, "ifname=%63s,", ifname) == 1 &&
                     s->down_script[0])
                     launch_script(s->down_script, ifname, s->fd);
             }