Patchwork [01/22] QemuOpts: fix a bug in QemuOpts when setting an option twice

login
register
mail settings
Submitter Anthony Liguori
Date June 7, 2010, 11:51 p.m.
Message ID <1275954730-8196-2-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/54897/
State New
Headers show

Comments

Anthony Liguori - June 7, 2010, 11:51 p.m.
Right now, if you set a QemuOpts option in a section twice, when you get the
option you'll receive the value that was set the first time.  This is less than
ideal because if you're manipulating options in two places like a global config
followed by the command line, you really want the later to override the former.

This patch fixes this behavior.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Gerd Hoffmann - June 8, 2010, 7:51 a.m.
On 06/08/10 01:51, Anthony Liguori wrote:
> Right now, if you set a QemuOpts option in a section twice, when you get the
> option you'll receive the value that was set the first time.  This is less than
> ideal because if you're manipulating options in two places like a global config
> followed by the command line, you really want the later to override the former.
>
> This patch fixes this behavior.

Note that this reverses the ordering for users which want multiple 
values (slirp forwarding for example).

cheers,
   Gerd
Paolo Bonzini - June 8, 2010, 10:32 a.m.
On 06/08/2010 09:51 AM, Gerd Hoffmann wrote:
> On 06/08/10 01:51, Anthony Liguori wrote:
>> Right now, if you set a QemuOpts option in a section twice, when
>> you get the option you'll receive the value that was set the first
>> time. This is less than ideal because if you're manipulating
>> options in two places like a global config followed by the command
>> line, you really want the later to override the former.
>>
>> This patch fixes this behavior.
>
> Note that this reverses the ordering for users which want multiple
> values (slirp forwarding for example).

And qemu_opt_find seems to have thought about this too:

static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
{
     QemuOpt *opt;

     QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
         if (strcmp(opt->name, name) != 0)
             continue;
         return opt;
     }
     return NULL;
}

Can you show the behavior with commandline arguments only?

Paolo
Anthony Liguori - June 8, 2010, 1:07 p.m.
On 06/08/2010 05:32 AM, Paolo Bonzini wrote:
> On 06/08/2010 09:51 AM, Gerd Hoffmann wrote:
>> On 06/08/10 01:51, Anthony Liguori wrote:
>>> Right now, if you set a QemuOpts option in a section twice, when
>>> you get the option you'll receive the value that was set the first
>>> time. This is less than ideal because if you're manipulating
>>> options in two places like a global config followed by the command
>>> line, you really want the later to override the former.
>>>
>>> This patch fixes this behavior.
>>
>> Note that this reverses the ordering for users which want multiple
>> values (slirp forwarding for example).
>
> And qemu_opt_find seems to have thought about this too:
>
> static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
> {
>     QemuOpt *opt;
>
>     QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
>         if (strcmp(opt->name, name) != 0)
>             continue;
>         return opt;
>     }
>     return NULL;
> }
>
> Can you show the behavior with commandline arguments only?

The problem I was trying to address can be seen with something like:

-drive file=foo.img,if=virtio,file=bar.img

You get no error, and foo.img is what gets used.  It's fair to argue 
this is a silly use case but what I'm trying to achieve is to make it 
possible to do:

-drive file=foo.img,if=virtio,id=bar -drive file=bar.img,id=bar

Or more specifically:

foo.conf:

[drive]
   file=foo.img
   if=virtio
   id=bar

qemu -readconfig foo.conf -drive file=bar.img,id=bar

IMHO, what's specified next on the command line ought to override what's 
in the config.

Suggestions how to achieve this in a more elegant way would be appreciated.

Regards,

Anthony Liguori

> Paolo
>
Gerd Hoffmann - June 8, 2010, 1:44 p.m.
On 06/08/10 15:07, Anthony Liguori wrote:
>>> Note that this reverses the ordering for users which want multiple
>>> values (slirp forwarding for example).
>>
>> And qemu_opt_find seems to have thought about this too:
>>
>> static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>> {
>> QemuOpt *opt;
>>
>> QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
>> if (strcmp(opt->name, name) != 0)
>> continue;
>> return opt;
>> }
>> return NULL;
>> }
>>
>> Can you show the behavior with commandline arguments only?
>
> The problem I was trying to address can be seen with something like:
>
> -drive file=foo.img,if=virtio,file=bar.img
>
> You get no error, and foo.img is what gets used.

Hmm.  I think qemuopts need to carry information about the option types, 
whenever it is single-entry or multiple-entry.

Oh, and likewise for the sections.  With multiple (unnamed) [device] 
sections we want create multiple (id-less) device sections.  With 
multiple [machine] sections we probably want to merge the options instead.

> Or more specifically:
>
> foo.conf:
>
> [drive]
> file=foo.img
> if=virtio
> id=bar

This would be '[drive "bar"]' without id= line btw.

> qemu -readconfig foo.conf -drive file=bar.img,id=bar

> IMHO, what's specified next on the command line ought to override what's
> in the config.

Or the user's config the global config.

For multi-entry options this will be tricky.  What do you expect to 
happen here:

global.conf
   [net "user"]
     type="slirp"
     guestfwd=<fw1>

user.conf
   [net "user"]
     guestfwd=<fw2>
     guestfwd=<fw3>

Which forwardings will be active then?  All of them?  Or will the 
user.conf forwardings override the global one?

cheers,
   Gerd
Paul Brook - June 8, 2010, 2:38 p.m.
> The problem I was trying to address can be seen with something like:
> 
> -drive file=foo.img,if=virtio,file=bar.img
> 
> You get no error, and foo.img is what gets used.  It's fair to argue
> this is a silly use case but what I'm trying to achieve is to make it
> possible to do:
> 
> -drive file=foo.img,if=virtio,id=bar -drive file=bar.img,id=bar

IMO these should both behave consistently. I'd prefer that both of are errors.

Paul
Anthony Liguori - June 8, 2010, 3:14 p.m.
On 06/08/2010 09:38 AM, Paul Brook wrote:
>> The problem I was trying to address can be seen with something like:
>>
>> -drive file=foo.img,if=virtio,file=bar.img
>>
>> You get no error, and foo.img is what gets used.  It's fair to argue
>> this is a silly use case but what I'm trying to achieve is to make it
>> possible to do:
>>
>> -drive file=foo.img,if=virtio,id=bar -drive file=bar.img,id=bar
>>      
> IMO these should both behave consistently. I'd prefer that both of are errors.
>    

It's fairly common that the last specified argument is what's 
respected.  For instance, if you do qemu -m 512M -m 1G, you'll get 1G of 
memory.

Regards,

Anthony Liguori

> Paul
>
Anthony Liguori - June 8, 2010, 3:17 p.m.
On 06/08/2010 08:44 AM, Gerd Hoffmann wrote:
> On 06/08/10 15:07, Anthony Liguori wrote:
>>>> Note that this reverses the ordering for users which want multiple
>>>> values (slirp forwarding for example).
>>>
>>> And qemu_opt_find seems to have thought about this too:
>>>
>>> static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>>> {
>>> QemuOpt *opt;
>>>
>>> QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) {
>>> if (strcmp(opt->name, name) != 0)
>>> continue;
>>> return opt;
>>> }
>>> return NULL;
>>> }
>>>
>>> Can you show the behavior with commandline arguments only?
>>
>> The problem I was trying to address can be seen with something like:
>>
>> -drive file=foo.img,if=virtio,file=bar.img
>>
>> You get no error, and foo.img is what gets used.
>
> Hmm.  I think qemuopts need to carry information about the option 
> types, whenever it is single-entry or multiple-entry.
>
> Oh, and likewise for the sections.  With multiple (unnamed) [device] 
> sections we want create multiple (id-less) device sections.  With 
> multiple [machine] sections we probably want to merge the options 
> instead.
>
>> Or more specifically:
>>
>> foo.conf:
>>
>> [drive]
>> file=foo.img
>> if=virtio
>> id=bar
>
> This would be '[drive "bar"]' without id= line btw.
>
>> qemu -readconfig foo.conf -drive file=bar.img,id=bar
>
>> IMHO, what's specified next on the command line ought to override what's
>> in the config.
>
> Or the user's config the global config.
>
> For multi-entry options this will be tricky.  What do you expect to 
> happen here:
>
> global.conf
>   [net "user"]
>     type="slirp"
>     guestfwd=<fw1>
>
> user.conf
>   [net "user"]
>     guestfwd=<fw2>
>     guestfwd=<fw3>
>
> Which forwardings will be active then?  All of them?  Or will the 
> user.conf forwardings override the global one?

What'd expect is:

[net "user"]
guestfwd = <fw1> <fw2> <fw3>..

I think multiple entry options are probably not a good thing to have.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
Gerd Hoffmann - June 8, 2010, 3:37 p.m.
> What'd expect is:
>
> [net "user"]
> guestfwd = <fw1> <fw2> <fw3>..
>
> I think multiple entry options are probably not a good thing to have.

We already have them though (-net switch so QemuOpts added them).

cheers,
   Gerd
Anthony Liguori - June 8, 2010, 4:04 p.m.
On 06/08/2010 10:37 AM, Gerd Hoffmann wrote:
>> What'd expect is:
>>
>> [net "user"]
>> guestfwd = <fw1> <fw2> <fw3>..
>>
>> I think multiple entry options are probably not a good thing to have.
>
> We already have them though (-net switch so QemuOpts added them).

Yeah, but let's ignore that for the moment.  If we didn't have 
historical baggage, would we prefer the syntax above or do you think 
there's value in specifying it on separate lines?

At this stage, I'm not worried about config file compatibility.  If we 
need to add some special logic to the -net switch to accommodate it's 
strange behavior, that's okay with me.

Regards,

Anthony Liguori

> cheers,
>   Gerd
Gerd Hoffmann - June 9, 2010, 7:01 a.m.
On 06/08/10 18:04, Anthony Liguori wrote:
> On 06/08/2010 10:37 AM, Gerd Hoffmann wrote:
>>> What'd expect is:
>>>
>>> [net "user"]
>>> guestfwd = <fw1> <fw2> <fw3>..
>>>
>>> I think multiple entry options are probably not a good thing to have.
>>
>> We already have them though (-net switch so QemuOpts added them).
>
> Yeah, but let's ignore that for the moment. If we didn't have historical
> baggage, would we prefer the syntax above or do you think there's value
> in specifying it on separate lines?

guestfwd entries are relatively long, so having them on one line is 
slightly annonying.  I'd tend to have them on separate lines.  But maybe 
it isn't a bad idea to extend the config file syntax to allow being more 
explicit here, like this (borrowed from makefile syntax):

   option := value       assign, overriding any existing value(s)
   option += value       append

Maybe also

   option ?= value       set only in case it isn't set yet.

cheers,
   Gerd

Patch

diff --git a/qemu-option.c b/qemu-option.c
index acd74f9..e0cb91b 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -628,7 +628,7 @@  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
     opt = qemu_mallocz(sizeof(*opt));
     opt->name = qemu_strdup(name);
     opt->opts = opts;
-    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
+    QTAILQ_INSERT_HEAD(&opts->head, opt, next);
     if (desc[i].name != NULL) {
         opt->desc = desc+i;
     }