Anthony Liguori <aliguori@xxxxxxxxxx> writes:
> Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> Date: Wed, 27 Feb 2013 09:32:09 -0600
> Subject: [PATCH] qemu-opts: support lists
>
> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
> ---
> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list