| Message ID | 874ngxhif9.fsf@codemonkey.ws |
|---|---|
| State | New |
| Headers | show |
Il 27/02/2013 16:42, Anthony Liguori ha scritto: > There's such thing as list support in QemuOpts. The only way > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > via options_int.h and rely on a implementation detail. > > There are fixed types supported by QemuOpts. It just so happens that > whenever qemu_opt_set() is called, instead of replacing the last > instance, the value is either prepended or appended in order to > implement a replace or set-if-unset behavior. Fair enough. Nobody said the implementation is pretty. > If we want to have list syntax, we need to introduce first class support > for it. Here's a simple example of how to do this. If it is meant as a prototype only, and the final command-line syntax would be with repeated keys, that's okay. I think that Eduardo/Markus/I are focusing on the user interface, you're focusing in the implementation. In the meanwhile, however, it seems to me that Eduardo can use QemuOptsVisitor---which can also hide the details and provide type safety. Paolo
On Wed, Feb 27, 2013 at 09:42:50AM -0600, Anthony Liguori wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 26/02/2013 20:35, Anthony Liguori ha scritto: > >>>> >> > >>>> >> See also discussion on multi-valued keys in command line option > >>>> >> arguments and config files in v1 thread. Hopefully we can reach a > >>>> >> conclusion soon, and then we'll see whether this patch is what we want. > >>> > > >>> > Yeah, let's drop this patch by now. I am starting to be convinced that > >>> > "cpus=A,cpus=B,cpus=C" is the best approach. It is not pretty, but at > >>> > least it uses generic parser code instead of yet another ad-hoc > >>> > parser. > >> No, we cannot rely on this behavior. We had to do it to support > >> backwards compat with netdev but it should not be used anywhere else. > > > > We chose a config format (git's) because it was a good match for > > QemuOpts, and multiple-valued operations are well supported by that > > config format; see git-config(1). There is no reason to consider it a > > backwards-compatibility hack. > > There's such thing as list support in QemuOpts. [skipping stuff about internal implementation details that don't matter] > > If we want to have list syntax, we need to introduce first class support > for it. Absolutely. But how the syntax for it should look like? > Here's a simple example of how to do this. Obviously we would > want to introduce some better error checking so we can catch if someone > tries to access a list with a non-list accessor. We also would need > list accessor functions. > > But it's really not hard at all. Of course. Once we decide how the syntax will look like, implementing it should be very easy. But as far as I can see, we were trying to discuss what's the appropriate syntax, here. I still don't see why "option=A:B:C" with no possibility of having list items containing ":" (like your proposal below) is a better generic syntax for lists than "option=A,option=B,option=C". > From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001 > From: Anthony Liguori <aliguori@us.ibm.com> > Date: Wed, 27 Feb 2013 09:32:09 -0600 > Subject: [PATCH] qemu-opts: support lists > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > include/qemu/option.h | 2 ++ > util/qemu-option.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/qemu/option.h b/include/qemu/option.h > index ba197cd..e4808c3 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -41,6 +41,7 @@ enum QEMUOptionParType { > typedef struct QEMUOptionParameter { > const char *name; > enum QEMUOptionParType type; > + bool list; > union { > uint64_t n; > char* s; > @@ -95,6 +96,7 @@ enum QemuOptType { > typedef struct QemuOptDesc { > const char *name; > enum QemuOptType type; > + bool list; > const char *help; > } QemuOptDesc; > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 5a1d03c..6b1ae6e 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, > return; > } > > + if (desc->list && strchr(value, ':')) { > + gchar **tokens = g_strsplit(value, ":", 0); > + int i; > + for (i = 0; tokens[i]; i++) { > + opt_set(opts, name, tokens[i], prepend, errp); > + if (error_is_set(errp)) { > + return; > + } > + } > + g_strfreev(tokens); > + } > + > opt = g_malloc0(sizeof(*opt)); > opt->name = g_strdup(name); > opt->opts = opts; > -- > 1.8.0 > > > Regards, > > Anthony Liguori > > > > > Paolo
On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: > Il 27/02/2013 16:42, Anthony Liguori ha scritto: > > There's such thing as list support in QemuOpts. The only way > > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > > via options_int.h and rely on a implementation detail. > > > > There are fixed types supported by QemuOpts. It just so happens that > > whenever qemu_opt_set() is called, instead of replacing the last > > instance, the value is either prepended or appended in order to > > implement a replace or set-if-unset behavior. > > Fair enough. Nobody said the implementation is pretty. > > > If we want to have list syntax, we need to introduce first class support > > for it. Here's a simple example of how to do this. > > If it is meant as a prototype only, and the final command-line syntax > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > are focusing on the user interface, you're focusing in the implementation. > > In the meanwhile, however, it seems to me that Eduardo can use > QemuOptsVisitor---which can also hide the details and provide type safety. Whatever I use to implement it, I still need to know how the command-line syntax will look like, because we need to tell libvirt developers how they should write the QEMU command-line.
Anthony Liguori <aliguori@us.ibm.com> writes: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 26/02/2013 20:35, Anthony Liguori ha scritto: >>>>> >> >>>>> >> See also discussion on multi-valued keys in command line option >>>>> >> arguments and config files in v1 thread. Hopefully we can reach a >>>>> >> conclusion soon, and then we'll see whether this patch is what we want. >>>> > >>>> > Yeah, let's drop this patch by now. I am starting to be convinced that >>>> > "cpus=A,cpus=B,cpus=C" is the best approach. It is not pretty, but at >>>> > least it uses generic parser code instead of yet another ad-hoc >>>> > parser. >>> No, we cannot rely on this behavior. We had to do it to support >>> backwards compat with netdev but it should not be used anywhere else. >> >> We chose a config format (git's) because it was a good match for >> QemuOpts, and multiple-valued operations are well supported by that >> config format; see git-config(1). There is no reason to consider it a >> backwards-compatibility hack. > > There's such thing as list support in QemuOpts. The only way > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > via options_int.h and rely on a implementation detail. > > There are fixed types supported by QemuOpts. It just so happens that > whenever qemu_opt_set() is called, instead of replacing the last > instance, the value is either prepended or appended in order to > implement a replace or set-if-unset behavior. > > All values are treated this way. So the above "list" would only be: > > { > .name = "cpus", > .type = QEMU_OPT_STRING > } > > Which is indistinguishable from a straight string property. This means > it's impossible to introspect because the type is context-sensitive. > > What's more, there is no API outside of QemuOptsVisitor that can > actually work with "lists" of QemuOpts values. There is: qemu_opt_foreach() If you relax your claim to "QemuOpts API for lists sucks and needs improvement", then we're in violent agreement. > If we want to have list syntax, we need to introduce first class support > for it. Here's a simple example of how to do this. Obviously we would > want to introduce some better error checking so we can catch if someone > tries to access a list with a non-list accessor. We also would need > list accessor functions. > > But it's really not hard at all. > > (Only compile tested) > > >>From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001 > From: Anthony Liguori <aliguori@us.ibm.com> > Date: Wed, 27 Feb 2013 09:32:09 -0600 > Subject: [PATCH] qemu-opts: support lists > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > include/qemu/option.h | 2 ++ > util/qemu-option.c | 12 ++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/include/qemu/option.h b/include/qemu/option.h > index ba197cd..e4808c3 100644 > --- a/include/qemu/option.h > +++ b/include/qemu/option.h > @@ -41,6 +41,7 @@ enum QEMUOptionParType { > typedef struct QEMUOptionParameter { > const char *name; > enum QEMUOptionParType type; > + bool list; > union { > uint64_t n; > char* s; > @@ -95,6 +96,7 @@ enum QemuOptType { > typedef struct QemuOptDesc { > const char *name; > enum QemuOptType type; > + bool list; > const char *help; > } QemuOptDesc; > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index 5a1d03c..6b1ae6e 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, > return; > } > > + if (desc->list && strchr(value, ':')) { > + gchar **tokens = g_strsplit(value, ":", 0); > + int i; > + for (i = 0; tokens[i]; i++) { > + opt_set(opts, name, tokens[i], prepend, errp); > + if (error_is_set(errp)) { > + return; > + } > + } > + g_strfreev(tokens); > + } > + > opt = g_malloc0(sizeof(*opt)); > opt->name = g_strdup(name); > opt->opts = opts; No, no, no. This makes ':' special, which means you can't have lists of anything containing ':'. Your cure is worse than the disease. Let go of that syntactic high-fructose corn syrup, stick to what we have and works just fine, thank you. Then add suitable list accessor functions and error checks.
Il 27/02/2013 17:19, Eduardo Habkost ha scritto: >> > >> > If it is meant as a prototype only, and the final command-line syntax >> > would be with repeated keys, that's okay. I think that Eduardo/Markus/I >> > are focusing on the user interface, you're focusing in the implementation. >> > >> > In the meanwhile, however, it seems to me that Eduardo can use >> > QemuOptsVisitor---which can also hide the details and provide type safety. > Whatever I use to implement it, I still need to know how the > command-line syntax will look like, because we need to tell libvirt > developers how they should write the QEMU command-line. I don't think any syntax makes sense except cpus=A,cpus=B. How we implement it is another story. Paolo
On Wed, Feb 27, 2013 at 05:58:39PM +0100, Paolo Bonzini wrote: > Il 27/02/2013 17:19, Eduardo Habkost ha scritto: > >> > > >> > If it is meant as a prototype only, and the final command-line syntax > >> > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > >> > are focusing on the user interface, you're focusing in the implementation. > >> > > >> > In the meanwhile, however, it seems to me that Eduardo can use > >> > QemuOptsVisitor---which can also hide the details and provide type safety. > > Whatever I use to implement it, I still need to know how the > > command-line syntax will look like, because we need to tell libvirt > > developers how they should write the QEMU command-line. > > I don't think any syntax makes sense except cpus=A,cpus=B. How we > implement it is another story. I agree completely, and I still don't know why Anthony doesn't like it.
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 27/02/2013 16:42, Anthony Liguori ha scritto: >> There's such thing as list support in QemuOpts. The only way >> QemuOptsVisitor was able to implement it was to expose QemuOpts publicly >> via options_int.h and rely on a implementation detail. >> >> There are fixed types supported by QemuOpts. It just so happens that >> whenever qemu_opt_set() is called, instead of replacing the last >> instance, the value is either prepended or appended in order to >> implement a replace or set-if-unset behavior. > > Fair enough. Nobody said the implementation is pretty. > >> If we want to have list syntax, we need to introduce first class support >> for it. Here's a simple example of how to do this. > > If it is meant as a prototype only, and the final command-line syntax > would be with repeated keys, that's okay. I think that Eduardo/Markus/I > are focusing on the user interface, you're focusing in the > implementation. No, I'm primarily motivated by the UI and am assuming that ya'll are arguing based on the implementation today. Repeating keys is quite mad. Here are some examples: qemu -numa node=1,node=2,cpus=2,cpus=3 What would a user expect that that would mean. What about: [numa] node=1 cpus=2 cpus=3 qemu -readconfig numa.cfg -numa node=1,cpus=1 I'm pretty sure you won't be able to say without looking at the source code. Now what about ranges? I'm pretty sure that what a user really wants to say is: qemu -numa node=1,cpus=0-3:8-11 This is easy to do with the patch I sent. We can add range support integer lists by just adding a check for '-' before doing dispatch. That's a heck of a lot nicer than: qemu -numa node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 With respect to escaping, for string lists (the only place where this point is even relevant), we can simply support escaping. But I'd like to hear a use-case for a string list first. Regards, Anthony Liguori > > In the meanwhile, however, it seems to me that Eduardo can use > QemuOptsVisitor---which can also hide the details and provide type safety. > > Paolo
Eduardo Habkost <ehabkost@redhat.com> writes: > On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: >> Il 27/02/2013 16:42, Anthony Liguori ha scritto: >> > There's such thing as list support in QemuOpts. The only way >> > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly >> > via options_int.h and rely on a implementation detail. >> > >> > There are fixed types supported by QemuOpts. It just so happens that >> > whenever qemu_opt_set() is called, instead of replacing the last >> > instance, the value is either prepended or appended in order to >> > implement a replace or set-if-unset behavior. >> >> Fair enough. Nobody said the implementation is pretty. >> >> > If we want to have list syntax, we need to introduce first class support >> > for it. Here's a simple example of how to do this. >> >> If it is meant as a prototype only, and the final command-line syntax >> would be with repeated keys, that's okay. I think that Eduardo/Markus/I >> are focusing on the user interface, you're focusing in the implementation. >> >> In the meanwhile, however, it seems to me that Eduardo can use >> QemuOptsVisitor---which can also hide the details and provide type safety. > > Whatever I use to implement it, I still need to know how the > command-line syntax will look like, because we need to tell libvirt > developers how they should write the QEMU command-line. Command line syntax is not committed until it appears in a release. libvirt *should not* assume any specific syntax until the 1.5 release ships. Regards, Anthony Liguori > > -- > Eduardo
Markus Armbruster <armbru@redhat.com> writes: > Anthony Liguori <aliguori@us.ibm.com> writes: > >> Which is indistinguishable from a straight string property. This means >> it's impossible to introspect because the type is context-sensitive. >> >> What's more, there is no API outside of QemuOptsVisitor that can >> actually work with "lists" of QemuOpts values. > > There is: qemu_opt_foreach() I'm not sure I believe that you wrote that with a straight face... ;-) >> opt = g_malloc0(sizeof(*opt)); >> opt->name = g_strdup(name); >> opt->opts = opts; > > No, no, no. This makes ':' special, which means you can't have lists of > anything containing ':'. Your cure is worse than the disease. Let go > of that syntactic high-fructose corn syrup, stick to what we have and > works just fine, thank you. Yes, there *must* be special syntax. If we're treating something special, then we should indicate to the user that it's special. Specifically, a list of integers should look distinctly different than overriding a previously specified integer. Regards, Anthony Liguori > > Then add suitable list accessor functions and error checks.
Il 27/02/2013 18:08, Anthony Liguori ha scritto: >> > >> > No, no, no. This makes ':' special, which means you can't have lists of >> > anything containing ':'. Your cure is worse than the disease. Let go >> > of that syntactic high-fructose corn syrup, stick to what we have and >> > works just fine, thank you. > Yes, there *must* be special syntax. If we're treating something > special, then we should indicate to the user that it's special. > > Specifically, a list of integers should look distinctly different than > overriding a previously specified integer. The solution is "there is no way to override a previously specified key". Something like "-device virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an error instead. Paolo
On Wed, Feb 27, 2013 at 11:04:08AM -0600, Anthony Liguori wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > > > On Wed, Feb 27, 2013 at 04:57:15PM +0100, Paolo Bonzini wrote: > >> Il 27/02/2013 16:42, Anthony Liguori ha scritto: > >> > There's such thing as list support in QemuOpts. The only way > >> > QemuOptsVisitor was able to implement it was to expose QemuOpts publicly > >> > via options_int.h and rely on a implementation detail. > >> > > >> > There are fixed types supported by QemuOpts. It just so happens that > >> > whenever qemu_opt_set() is called, instead of replacing the last > >> > instance, the value is either prepended or appended in order to > >> > implement a replace or set-if-unset behavior. > >> > >> Fair enough. Nobody said the implementation is pretty. > >> > >> > If we want to have list syntax, we need to introduce first class support > >> > for it. Here's a simple example of how to do this. > >> > >> If it is meant as a prototype only, and the final command-line syntax > >> would be with repeated keys, that's okay. I think that Eduardo/Markus/I > >> are focusing on the user interface, you're focusing in the implementation. > >> > >> In the meanwhile, however, it seems to me that Eduardo can use > >> QemuOptsVisitor---which can also hide the details and provide type safety. > > > > Whatever I use to implement it, I still need to know how the > > command-line syntax will look like, because we need to tell libvirt > > developers how they should write the QEMU command-line. > > Command line syntax is not committed until it appears in a release. > libvirt *should not* assume any specific syntax until the 1.5 release > ships. I am just talking about communication with libvirt developers. Developers surely write code and test their work in progress using unreleased QEMU code, instead of waiting for an official release. Nobody suggested releasing a libvirt version that relies on an unreleased QEMU feature.
On 02/27/2013 10:04 AM, Anthony Liguori wrote: >> Whatever I use to implement it, I still need to know how the >> command-line syntax will look like, because we need to tell libvirt >> developers how they should write the QEMU command-line. > > Command line syntax is not committed until it appears in a release. > libvirt *should not* assume any specific syntax until the 1.5 release > ships. Libvirt 1.0.3 will already error out on any disjoint ranges, and we are just fine waiting until qemu 1.5 is released before adding any code to support disjoint ranges in the preferred syntax. Qemu should feel free to implement it correctly, rather than trying to cater to older (broken) libvirt's abuse of comma.
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 27/02/2013 18:08, Anthony Liguori ha scritto: >>> > >>> > No, no, no. This makes ':' special, which means you can't have lists of >>> > anything containing ':'. Your cure is worse than the disease. Let go >>> > of that syntactic high-fructose corn syrup, stick to what we have and >>> > works just fine, thank you. >> Yes, there *must* be special syntax. If we're treating something >> special, then we should indicate to the user that it's special. >> >> Specifically, a list of integers should look distinctly different than >> overriding a previously specified integer. > > The solution is "there is no way to override a previously specified > key". Something like "-device > virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an > error instead. That breaks compatibility. The above may seem silly but consider: qemu -device virtio-scsi-pci,num_queues=1,id=foo \ -set device.foo.num_queues=2 This is more common than you would think primarily as a way to override options that libvirt has set either via the qemu extra args tag or a script wrapper of qemu. Regards, Anthony Liguori > > Paolo
Il 27/02/2013 18:38, Anthony Liguori ha scritto: >> > The solution is "there is no way to override a previously specified >> > key". Something like "-device >> > virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an >> > error instead. > That breaks compatibility. The above may seem silly but consider: > > qemu -device virtio-scsi-pci,num_queues=1,id=foo \ > -set device.foo.num_queues=2 > > This is more common than you would think primarily as a way to override > options that libvirt has set either via the qemu extra args tag or a > script wrapper of qemu. "-set" could first delete a pre-existing option. We could also extend "-set" to accept multiple values (like -set device.foo.bar=baz,device.foo.qux=quux), which is useful also to set a list option. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 27/02/2013 17:19, Eduardo Habkost ha scritto: >>> > >>> > If it is meant as a prototype only, and the final command-line syntax >>> > would be with repeated keys, that's okay. I think that Eduardo/Markus/I >>> > are focusing on the user interface, you're focusing in the implementation. >>> > >>> > In the meanwhile, however, it seems to me that Eduardo can use >>> > QemuOptsVisitor---which can also hide the details and provide type safety. >> Whatever I use to implement it, I still need to know how the >> command-line syntax will look like, because we need to tell libvirt >> developers how they should write the QEMU command-line. > > I don't think any syntax makes sense except cpus=A,cpus=B. How we > implement it is another story. Agreed on both counts.
Anthony Liguori <anthony@codemonkey.ws> writes: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 27/02/2013 16:42, Anthony Liguori ha scritto: >>> There's such thing as list support in QemuOpts. The only way >>> QemuOptsVisitor was able to implement it was to expose QemuOpts publicly >>> via options_int.h and rely on a implementation detail. >>> >>> There are fixed types supported by QemuOpts. It just so happens that >>> whenever qemu_opt_set() is called, instead of replacing the last >>> instance, the value is either prepended or appended in order to >>> implement a replace or set-if-unset behavior. >> >> Fair enough. Nobody said the implementation is pretty. >> >>> If we want to have list syntax, we need to introduce first class support >>> for it. Here's a simple example of how to do this. >> >> If it is meant as a prototype only, and the final command-line syntax >> would be with repeated keys, that's okay. I think that Eduardo/Markus/I >> are focusing on the user interface, you're focusing in the >> implementation. > > No, I'm primarily motivated by the UI and am assuming that ya'll are > arguing based on the implementation today. Your assumption is incorrect :) > Repeating keys is quite > mad. Here are some examples: > > qemu -numa node=1,node=2,cpus=2,cpus=3 > > What would a user expect that that would mean. I figure you mean -numa node,nodeid=1,nodeid=2,cpus=2,cpus=3 Parameter nodeid is int-valued, therefore repeating it is either an error, or the last one wins. Matter of taste. Currently, we do the latter. Parameter cpus is list-valued, therefore the values accumulate. We already do that. Taken together, this configures node 1 to use cpus 2 and 3. > What about: > > [numa] > node=1 > cpus=2 > cpus=3 > > qemu -readconfig numa.cfg -numa node=1,cpus=1 I figure you mean [numa] nodeid=1 cpus=2 cpus=3 qemu -readconfig numa.cfg -numa node,nodeid=1,cpus=1 The config file configures node 1 to use cpus 2 and 3. The command line configures node 1 to use cpus 1. QemuOpts can optionally be made to merge options with the same ID. We use that in cases like -machine, where multiple options make no sense, but merging them does. Merging options doesn't make sense for -numa. Therefore, configuration file and command line are *not* merged. In particular, the two cpus lists are not merged. So, what we have is two conflicting bits of configuration for the same thing. That's not a new problem, we got that everywhere. Either treat as error, or let the last one win. Matter of taste. Currently, we do the latter. > I'm pretty sure you won't be able to say without looking at the source > code. I hereby certify that I did not look at the source code, only at help output. So there. > Now what about ranges? I'm pretty sure that what a user really wants to > say is: > > qemu -numa node=1,cpus=0-3:8-11 > > This is easy to do with the patch I sent. We can add range support > integer lists by just adding a check for '-' before doing dispatch. > That's a heck of a lot nicer than: > > qemu -numa > node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 Let me pick up the baby you just threw out with the bathwater for you: qemu -numa node,nodeid=1,cpus=0-3,cpus=8-11 > With respect to escaping, for string lists (the only place where this > point is even relevant), we can simply support escaping. But I'd like > to hear a use-case for a string list first. There is no need for the syntactic warts you so cavalierly propose. Whenever you do key=X:Y, I can do key=X,key=Y. Whatever semantics you propose for the former, I'll propose for the latter.
Anthony Liguori <anthony@codemonkey.ws> writes: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 27/02/2013 18:08, Anthony Liguori ha scritto: >>>> > >>>> > No, no, no. This makes ':' special, which means you can't have lists of >>>> > anything containing ':'. Your cure is worse than the disease. Let go >>>> > of that syntactic high-fructose corn syrup, stick to what we have and >>>> > works just fine, thank you. >>> Yes, there *must* be special syntax. If we're treating something >>> special, then we should indicate to the user that it's special. >>> >>> Specifically, a list of integers should look distinctly different than >>> overriding a previously specified integer. >> >> The solution is "there is no way to override a previously specified >> key". Something like "-device >> virtio-scsi-pci,num_queues=1,num_queues=2" now works, let's make it an >> error instead. > > That breaks compatibility. The above may seem silly but consider: > > qemu -device virtio-scsi-pci,num_queues=1,id=foo \ > -set device.foo.num_queues=2 > > This is more common than you would think primarily as a way to override > options that libvirt has set either via the qemu extra args tag or a > script wrapper of qemu. Related: overwrite something you got from a config file on the command line. In both your example and mine, we have entirely separate options, and they have perfectly ordinary overwrite semantics: each option overwrites the given keys with the given values. The last key=value wins. This usage makes sense, and we obviously want to preserve it. Paolo's example is different only in that it's a silly. Preserving compatibility may mean that once we accepted silly usage, we can't ever reject it. Debatable. Personally, I disagree: I think we could outlaw repeating keys within the same option argument / configuration file section just fine. Finally, I don't think that we must have fancy-pants syntax to remind users that they're configuring a list. What's the chance of confusion there? What user would expect num_queues=1,num_queues=2 to make num_queues magically become a list?
Markus Armbruster <armbru@redhat.com> writes: > Anthony Liguori <anthony@codemonkey.ws> writes: > >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> What about: >> >> [numa] >> node=1 >> cpus=2 >> cpus=3 >> >> qemu -readconfig numa.cfg -numa node=1,cpus=1 > > I figure you mean > > [numa] > nodeid=1 > cpus=2 > cpus=3 > > qemu -readconfig numa.cfg -numa node,nodeid=1,cpus=1 > > The config file configures node 1 to use cpus 2 and 3. > > The command line configures node 1 to use cpus 1. > > QemuOpts can optionally be made to merge options with the same ID. We > use that in cases like -machine, where multiple options make no sense, > but merging them does. With QemuOpts, this is treated as two distinct sections with anonymous ids so whatever the code is to handle NUMA options would get called twice and it's up to that code to determine how it handles the conflict. (This is admittedly irrelevant to the discussion.) > Merging options doesn't make sense for -numa. Therefore, configuration > file and command line are *not* merged. In particular, the two cpus > lists are not merged. > > So, what we have is two conflicting bits of configuration for the same > thing. That's not a new problem, we got that everywhere. Either treat > as error, or let the last one win. Matter of taste. Currently, we do > the latter. > >> I'm pretty sure you won't be able to say without looking at the source >> code. > > I hereby certify that I did not look at the source code, only at help > output. So there. > >> Now what about ranges? I'm pretty sure that what a user really wants to >> say is: >> >> qemu -numa node=1,cpus=0-3:8-11 >> >> This is easy to do with the patch I sent. We can add range support >> integer lists by just adding a check for '-' before doing dispatch. >> That's a heck of a lot nicer than: >> >> qemu -numa >> node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 > > Let me pick up the baby you just threw out with the bathwater for you: > > qemu -numa node,nodeid=1,cpus=0-3,cpus=8-11 If you're okay with making '-' be special syntax, why are you not okay with ':' being special syntax? What I assume your proposing is making cpus be a string list and then parsing within the NUMA code. Why not do it all in QemuOpts core code? Regards, Anthony Liguori > >> With respect to escaping, for string lists (the only place where this >> point is even relevant), we can simply support escaping. But I'd like >> to hear a use-case for a string list first. > > There is no need for the syntactic warts you so cavalierly propose. > Whenever you do key=X:Y, I can do key=X,key=Y. Whatever semantics you > propose for the former, I'll propose for the latter.
Markus Armbruster <armbru@redhat.com> writes: > Related: overwrite something you got from a config file on the command > line. > > In both your example and mine, we have entirely separate options, and > they have perfectly ordinary overwrite semantics: each option overwrites > the given keys with the given values. The last key=value wins. > > This usage makes sense, and we obviously want to preserve it. > > Paolo's example is different only in that it's a silly. Preserving > compatibility may mean that once we accepted silly usage, we can't ever > reject it. Debatable. Personally, I disagree: I think we could outlaw > repeating keys within the same option argument / configuration file > section just fine. > > Finally, I don't think that we must have fancy-pants syntax to remind > users that they're configuring a list. What's the chance of confusion > there? What user would expect num_queues=1,num_queues=2 to make > num_queues magically become a list? My fundamental problem here is that we have the same syntax with different meanings depending on the context. Going back to our original example: qemu -numa node,nodeid=2,cpus=4 This is certainly ambiguous. Does this mean that you have a single cpu for the node (VCPU 4) or does it mean the node have 4 cpus (presumably ranged 0-3). Given that ambiguity the following: qemu -numa node,nodeid=2,cpus=4,cpus=8 Does help the situation. A reasonable person could assume that cpus=8 overrides the previous cpus=4 (as it does elsewhere in QEMU) and therefore assume they were creating a node with 8 CPUS (0-7) instead of two cpus. However: qemu -numa node,nodeid=2,cpus=4:8 Is much less ambiguous. Granted, it's not immediately obvious whether this is a range specification or a disjoint specification but it's more clear than the previous syntax. Regards, Anthony Liguori
Il 28/02/2013 14:32, Anthony Liguori ha scritto: >>> >> qemu -numa >>> >> node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 >> > >> > Let me pick up the baby you just threw out with the bathwater for you: >> > >> > qemu -numa node,nodeid=1,cpus=0-3,cpus=8-11 > If you're okay with making '-' be special syntax, why are you not okay > with ':' being special syntax? For example because another kind of string list could have a colon inside, and that would in turn need escaping (in fact that's already the case for guestfwd); repeating the key is a syntax that is easily reusable (and indeed is already in use). Instead, the '-' is parsed within the NUMA code, and is completely opaque to QemuOpts. The NUMA code knows that the '-' will never need escaping, because it only deals with positive integers. A perhaps better question would have been "if you're okay with making ',' be special syntax, why not ':'". And the answer is that indeed ',' already brings some problems, but likely they outweight the advantages of having say only a "-set" option. But adding a second escaped character is already much more debatable in my opinion. > What I assume your proposing is making cpus be a string list and then > parsing within the NUMA code. Why not do it all in QemuOpts core code? What would the QemuOpts parsing code do? Do you have in mind bitmasks as a first-class QemuOpts type? If so, that would be an argument in favor of ':', but against the prototype patch you posted yesterday. Paolo
Il 28/02/2013 14:41, Anthony Liguori ha scritto: > > This is certainly ambiguous. Does this mean that you have a single cpu > for the node (VCPU 4) or does it mean the node have 4 cpus (presumably > ranged 0-3). > > Given that ambiguity the following: > > qemu -numa node,nodeid=2,cpus=4,cpus=8 > > Does help the situation. A reasonable person could assume that cpus=8 > overrides the previous cpus=4 (as it does elsewhere in QEMU) and > therefore assume they were creating a node with 8 CPUS (0-7) instead of > two cpus. However: > > qemu -numa node,nodeid=2,cpus=4:8 > > Is much less ambiguous. Granted, it's not immediately obvious whether > this is a range specification or a disjoint specification but it's more > clear than the previous syntax. This makes your point clear, but it sounds a bit artificial. "4" or "8" would never appear alone. You would likely have something like -numa node,nodeid=0,cpus=0,cpus=12 \ -numa node,nodeid=1,cpus=1,cpus=13 \ -numa node,nodeid=2,cpus=2,cpus=14 \ -numa node,nodeid=3,cpus=3,cpus=15 \ -numa node,nodeid=4,cpus=4,cpus=8 which would make the syntax much more obvious. Something like 4:8 would be rather unclear actually, because both numbes are even. Given "-numa node,nodeid=2,cpus=4:8" out of context, I would guess that 4:8 is [4,8) where the upper-bound is excluded for some reason. Of course context would clear it up, but that also applies to cpus=foo,cpus=bar. Paolo
Anthony Liguori <anthony@codemonkey.ws> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Related: overwrite something you got from a config file on the command >> line. >> >> In both your example and mine, we have entirely separate options, and >> they have perfectly ordinary overwrite semantics: each option overwrites >> the given keys with the given values. The last key=value wins. >> >> This usage makes sense, and we obviously want to preserve it. >> >> Paolo's example is different only in that it's a silly. Preserving >> compatibility may mean that once we accepted silly usage, we can't ever >> reject it. Debatable. Personally, I disagree: I think we could outlaw >> repeating keys within the same option argument / configuration file >> section just fine. >> >> Finally, I don't think that we must have fancy-pants syntax to remind >> users that they're configuring a list. What's the chance of confusion >> there? What user would expect num_queues=1,num_queues=2 to make >> num_queues magically become a list? > > My fundamental problem here is that we have the same syntax with > different meanings depending on the context. > > Going back to our original example: > > qemu -numa node,nodeid=2,cpus=4 > > This is certainly ambiguous. Does this mean that you have a single cpu > for the node (VCPU 4) or does it mean the node have 4 cpus (presumably > ranged 0-3). Root cause: the name "cpus" can be interpreted as "number of cpus" or as "list of cpus". Fix (if it's worth fixing): use a better name. First one that crossed my mind: "cpu". > Given that ambiguity the following: > > qemu -numa node,nodeid=2,cpus=4,cpus=8 > > Does help the situation. A reasonable person could assume that cpus=8 > overrides the previous cpus=4 (as it does elsewhere in QEMU) and I suspect a reasonable person is blissfully unaware of the fact that he can give the same key several times in a single option argument, let alone what happens when he does. And I still think we could outlaw such repetition if we cared. Besides the command line, there's also the config file. As Paolo explained, "repeated key means list" is established practice there. > therefore assume they were creating a node with 8 CPUS (0-7) instead of > two cpus. However: > > qemu -numa node,nodeid=2,cpus=4:8 > > Is much less ambiguous. Granted, it's not immediately obvious whether > this is a range specification or a disjoint specification but it's more > clear than the previous syntax. Does it mean CPU 4 and 8? CPU 4 to 8? 8 CPUs starting with 4? If it's less ambiguous, then probably because it's sufficiently greek to make people reach for the manual :) Moreover, no change, thus no improvement for your original example "cpus=4", which you called "certainly ambigous".
Anthony Liguori <anthony@codemonkey.ws> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Anthony Liguori <anthony@codemonkey.ws> writes: >> >>> Paolo Bonzini <pbonzini@redhat.com> writes: >>> >>> What about: >>> >>> [numa] >>> node=1 >>> cpus=2 >>> cpus=3 >>> >>> qemu -readconfig numa.cfg -numa node=1,cpus=1 >> >> I figure you mean >> >> [numa] >> nodeid=1 >> cpus=2 >> cpus=3 >> >> qemu -readconfig numa.cfg -numa node,nodeid=1,cpus=1 >> >> The config file configures node 1 to use cpus 2 and 3. >> >> The command line configures node 1 to use cpus 1. >> >> QemuOpts can optionally be made to merge options with the same ID. We >> use that in cases like -machine, where multiple options make no sense, >> but merging them does. > > With QemuOpts, this is treated as two distinct sections with anonymous > ids so whatever the code is to handle NUMA options would get called > twice and it's up to that code to determine how it handles the > conflict. (This is admittedly irrelevant to the discussion.) Correct. >> Merging options doesn't make sense for -numa. Therefore, configuration >> file and command line are *not* merged. In particular, the two cpus >> lists are not merged. >> >> So, what we have is two conflicting bits of configuration for the same >> thing. That's not a new problem, we got that everywhere. Either treat >> as error, or let the last one win. Matter of taste. Currently, we do >> the latter. >> >>> I'm pretty sure you won't be able to say without looking at the source >>> code. >> >> I hereby certify that I did not look at the source code, only at help >> output. So there. >> >>> Now what about ranges? I'm pretty sure that what a user really wants to >>> say is: >>> >>> qemu -numa node=1,cpus=0-3:8-11 >>> >>> This is easy to do with the patch I sent. We can add range support >>> integer lists by just adding a check for '-' before doing dispatch. >>> That's a heck of a lot nicer than: >>> >>> qemu -numa >>> node=1,cpus=0,cpus=1,cpus=2,cpus=3,cpus=8,cpus=9,cpus=10,cpus=11 >> >> Let me pick up the baby you just threw out with the bathwater for you: >> >> qemu -numa node,nodeid=1,cpus=0-3,cpus=8-11 > > If you're okay with making '-' be special syntax, why are you not okay > with ':' being special syntax? Fair question! > What I assume your proposing is making cpus be a string list and then > parsing within the NUMA code. Why not do it all in QemuOpts core code? Yes, and another fair question. I want common QemuOpts syntax used instead of ad hoc syntax whenever practical, because ad hoc syntax is bound to breed inconsistencies and special cases. QemuOpts was created to get us out of that pit; let's not jump back in without a really good cause. We already got lists in QemuOpts. We don't have intervals in QemuOpts. If we had, I'd certainly argue for using them here. If more uses of intervals turn up, I'll argue for putting intervals in QemuOpts.
From 4ee7ed36d597f0defd78baac7480ecb29e11e1c1 Mon Sep 17 00:00:00 2001 From: Anthony Liguori <aliguori@us.ibm.com> Date: Wed, 27 Feb 2013 09:32:09 -0600 Subject: [PATCH] qemu-opts: support lists Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- include/qemu/option.h | 2 ++ util/qemu-option.c | 12 ++++++++++++ 2 files changed, 14 insertions(+) diff --git a/include/qemu/option.h b/include/qemu/option.h index ba197cd..e4808c3 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -41,6 +41,7 @@ enum QEMUOptionParType { typedef struct QEMUOptionParameter { const char *name; enum QEMUOptionParType type; + bool list; union { uint64_t n; char* s; @@ -95,6 +96,7 @@ enum QemuOptType { typedef struct QemuOptDesc { const char *name; enum QemuOptType type; + bool list; const char *help; } QemuOptDesc; diff --git a/util/qemu-option.c b/util/qemu-option.c index 5a1d03c..6b1ae6e 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -636,6 +636,18 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value, return; } + if (desc->list && strchr(value, ':')) { + gchar **tokens = g_strsplit(value, ":", 0); + int i; + for (i = 0; tokens[i]; i++) { + opt_set(opts, name, tokens[i], prepend, errp); + if (error_is_set(errp)) { + return; + } + } + g_strfreev(tokens); + } + opt = g_malloc0(sizeof(*opt)); opt->name = g_strdup(name); opt->opts = opts; -- 1.8.0