| Message ID | 1275954730-8196-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 >
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
> 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
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 >
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 >
> 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
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
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
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; }
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>