Patchwork [v2] vl.c: Support multiple CPU ranges on -numa option

login
register
mail settings
Submitter Anthony Liguori
Date Feb. 27, 2013, 3:42 p.m.
Message ID <874ngxhif9.fsf@codemonkey.ws>
Download mbox | patch
Permalink /patch/223640/
State New
Headers show

Comments

Anthony Liguori - Feb. 27, 2013, 3:42 p.m.
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.

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)
Regards,

Anthony Liguori

>
> Paolo
Paolo Bonzini - Feb. 27, 2013, 3:57 p.m.
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
Eduardo Habkost - Feb. 27, 2013, 4:13 p.m.
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
Eduardo Habkost - Feb. 27, 2013, 4:19 p.m.
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.
Markus Armbruster - Feb. 27, 2013, 4:23 p.m.
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.
Paolo Bonzini - Feb. 27, 2013, 4:58 p.m.
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
Eduardo Habkost - Feb. 27, 2013, 5:02 p.m.
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.
Anthony Liguori - Feb. 27, 2013, 5:02 p.m.
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
Anthony Liguori - Feb. 27, 2013, 5:04 p.m.
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
Anthony Liguori - Feb. 27, 2013, 5:08 p.m.
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.
Paolo Bonzini - Feb. 27, 2013, 5:17 p.m.
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
Eduardo Habkost - Feb. 27, 2013, 5:22 p.m.
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.
Eric Blake - Feb. 27, 2013, 5:37 p.m.
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.
Anthony Liguori - Feb. 27, 2013, 5:38 p.m.
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
Paolo Bonzini - Feb. 27, 2013, 5:51 p.m.
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
Markus Armbruster - Feb. 28, 2013, 12:29 p.m.
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.
Markus Armbruster - Feb. 28, 2013, 12:48 p.m.
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.
Markus Armbruster - Feb. 28, 2013, 1:09 p.m.
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?
Anthony Liguori - Feb. 28, 2013, 1:32 p.m.
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.
Anthony Liguori - Feb. 28, 2013, 1:41 p.m.
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
Paolo Bonzini - Feb. 28, 2013, 3:05 p.m.
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
Paolo Bonzini - Feb. 28, 2013, 3:12 p.m.
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
Markus Armbruster - Feb. 28, 2013, 5:34 p.m.
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".
Markus Armbruster - Feb. 28, 2013, 5:41 p.m.
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.

Patch

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