[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Qemu-devel] [PATCH v2] vl.c: Support multiple CPU ranges on -numa option



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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]