[Qemu-devel] [PATCH v2 0/8]: rename machine options to use dashes

[Qemu-devel] [PATCH v2 0/8]: rename machine options to use dashes

[Luiz Capitulino]

Today, machine options use underscores to separate words (eg. kernel_irqchip),
however upcoming QOM conversion wants to use dashes instead.

This series converts all machine type options to use dashes. Command-line
backwards compatibility is maintained by adding an alias for each changed
option.

The first half of the series add alias support to qemu-option. The second
half does the machine type options conversion.

v2

o fix code duplication in qemu_opt_set_bool() and qemu_opts_validate()
  (this automatically makes them support aliases) [Markus]
o re-work commit logs

 device_tree.c          |  2 +-
 hw/ppce500_mpc8544ds.c |  2 +-
 kvm-all.c              |  2 +-
 qemu-config.c          | 12 ++++---
 qemu-option.c          | 87 ++++++++++++++++++--------------------------------
 qemu-option.h          |  1 +
 qemu-options.hx        |  8 ++---
 target-i386/kvm.c      |  2 +-
 8 files changed, 48 insertions(+), 68 deletions(-)

[Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Luiz Capitulino]

Call qemu_opt_set() instead of duplicating opt_set().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c | 28 +---------------------------
 1 file changed, 1 insertion(+), 27 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index bb3886c..2cb2835 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
 
 int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
 {
-    QemuOpt *opt;
-    const QemuOptDesc *desc = opts->list->desc;
-    int i;
-
-    for (i = 0; desc[i].name != NULL; i++) {
-        if (strcmp(desc[i].name, name) == 0) {
-            break;
-        }
-    }
-    if (desc[i].name == NULL) {
-        if (i == 0) {
-            /* empty list -> allow any */;
-        } else {
-            qerror_report(QERR_INVALID_PARAMETER, name);
-            return -1;
-        }
-    }
-
-    opt = g_malloc0(sizeof(*opt));
-    opt->name = g_strdup(name);
-    opt->opts = opts;
-    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-    if (desc[i].name != NULL) {
-        opt->desc = desc+i;
-    }
-    opt->value.boolean = !!val;
-    return 0;
+    return qemu_opt_set(opts, name, val ? "on" : "off");
 }
 
 int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 2/8] qemu-option: opt_set(): split it up into more functions

[Luiz Capitulino]

The new functions are opts_accepts_any() and find_desc_by_name(), which
are also going to be used by qemu_opts_validate() (see next commit).

This also makes opt_set() slightly more readable.
---
 qemu-option.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 2cb2835..d67e10f 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -612,26 +612,36 @@ static void qemu_opt_del(QemuOpt *opt)
     g_free(opt);
 }
 
-static void opt_set(QemuOpts *opts, const char *name, const char *value,
-                    bool prepend, Error **errp)
+static bool opts_accepts_any(const QemuOpts *opts)
+{
+    return opts->list->desc[0].name == NULL;
+}
+
+static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
+                                            const char *name)
 {
-    QemuOpt *opt;
-    const QemuOptDesc *desc = opts->list->desc;
-    Error *local_err = NULL;
     int i;
 
     for (i = 0; desc[i].name != NULL; i++) {
         if (strcmp(desc[i].name, name) == 0) {
-            break;
+            return &desc[i];
         }
     }
-    if (desc[i].name == NULL) {
-        if (i == 0) {
-            /* empty list -> allow any */;
-        } else {
-            error_set(errp, QERR_INVALID_PARAMETER, name);
-            return;
-        }
+
+    return NULL;
+}
+
+static void opt_set(QemuOpts *opts, const char *name, const char *value,
+                    bool prepend, Error **errp)
+{
+    QemuOpt *opt;
+    const QemuOptDesc *desc;
+    Error *local_err = NULL;
+
+    desc = find_desc_by_name(opts->list->desc, name);
+    if (!desc && !opts_accepts_any(opts)) {
+        error_set(errp, QERR_INVALID_PARAMETER, name);
+        return;
     }
 
     opt = g_malloc0(sizeof(*opt));
@@ -642,9 +652,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
     } else {
         QTAILQ_INSERT_TAIL(&opts->head, opt, next);
     }
-    if (desc[i].name != NULL) {
-        opt->desc = desc+i;
-    }
+    opt->desc = desc;
     if (value) {
         opt->str = g_strdup(value);
     }
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 3/8] qemu-option: qemu_opts_validate(): fix duplicated code

[Luiz Capitulino]

Use opts_accepts_any() and find_desc_by_name().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index d67e10f..65ba1cf 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -1060,23 +1060,15 @@ void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp)
     QemuOpt *opt;
     Error *local_err = NULL;
 
-    assert(opts->list->desc[0].name == NULL);
+    assert(opts_accepts_any(opts));
 
     QTAILQ_FOREACH(opt, &opts->head, next) {
-        int i;
-
-        for (i = 0; desc[i].name != NULL; i++) {
-            if (strcmp(desc[i].name, opt->name) == 0) {
-                break;
-            }
-        }
-        if (desc[i].name == NULL) {
+        opt->desc = find_desc_by_name(desc, opt->name);
+        if (!opt->desc) {
             error_set(errp, QERR_INVALID_PARAMETER, opt->name);
             return;
         }
 
-        opt->desc = &desc[i];
-
         qemu_opt_parse(opt, &local_err);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

Allow for specifying an alias for each option name, see next commits
for examples.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-option.c | 5 +++--
 qemu-option.h | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/qemu-option.c b/qemu-option.c
index 65ba1cf..b2f9e21 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
     int i;
 
     for (i = 0; desc[i].name != NULL; i++) {
-        if (strcmp(desc[i].name, name) == 0) {
+        if (strcmp(desc[i].name, name) == 0 ||
+            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
             return &desc[i];
         }
     }
@@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
     }
 
     opt = g_malloc0(sizeof(*opt));
-    opt->name = g_strdup(name);
+    opt->name = g_strdup(desc ? desc->name : name);
     opt->opts = opts;
     if (prepend) {
         QTAILQ_INSERT_HEAD(&opts->head, opt, next);
diff --git a/qemu-option.h b/qemu-option.h
index 951dec3..7106d2f 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -94,6 +94,7 @@ enum QemuOptType {
 
 typedef struct QemuOptDesc {
     const char *name;
+    const char *alias;
     enum QemuOptType type;
     const char *help;
 } QemuOptDesc;
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 5/8] machine: rename kernel_irqchip to kernel-irqchip

[Luiz Capitulino]

QOM conversion wants option names with dashes. This commit does the
change for the kernel_irqchip machine type option.

The old option name is still supported through an option alias for
backwards compatibility.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 kvm-all.c       | 2 +-
 qemu-config.c   | 3 ++-
 qemu-options.hx | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index f8e4328..c988bb6 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1168,7 +1168,7 @@ static int kvm_irqchip_create(KVMState *s)
 
     if (QTAILQ_EMPTY(&list->head) ||
         !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
-                           "kernel_irqchip", true) ||
+                           "kernel-irqchip", true) ||
         !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
         return 0;
     }
diff --git a/qemu-config.c b/qemu-config.c
index 5c3296b..3fe91a8 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -560,7 +560,8 @@ static QemuOptsList qemu_machine_opts = {
             .type = QEMU_OPT_STRING,
             .help = "accelerator list",
         }, {
-            .name = "kernel_irqchip",
+            .name = "kernel-irqchip",
+            .alias= "kernel_irqchip",
             .type = QEMU_OPT_BOOL,
             .help = "use KVM in-kernel irqchip",
         }, {
diff --git a/qemu-options.hx b/qemu-options.hx
index ecf7ca1..b149852 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -32,7 +32,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                selects emulated machine (-machine ? for list)\n"
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
     "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
-    "                kernel_irqchip=on|off controls accelerated irqchip support\n"
+    "                kernel-irqchip=on|off controls accelerated irqchip support\n"
     "                kvm_shadow_mem=size of KVM shadow MMU\n",
     QEMU_ARCH_ALL)
 STEXI
@@ -46,7 +46,7 @@ This is used to enable an accelerator. Depending on the target architecture,
 kvm, xen, or tcg can be available. By default, tcg is used. If there is more
 than one accelerator specified, the next one is used if the previous one fails
 to initialize.
-@item kernel_irqchip=on|off
+@item kernel-irqchip=on|off
 Enables in-kernel irqchip support for the chosen accelerator when available.
 @item kvm_shadow_mem=size
 Defines the size of the KVM shadow MMU.
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 6/8] machine: rename kvm_shadow_mem to kvm-shadow-mem

[Luiz Capitulino]

QOM conversion wants option names with dashes. This commit does the
change for the kvm_shadow_mem machine type option.

The old option name is still supported through an option alias for
backwards compatibility.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 qemu-config.c     | 3 ++-
 qemu-options.hx   | 4 ++--
 target-i386/kvm.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 3fe91a8..909fae9 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -565,7 +565,8 @@ static QemuOptsList qemu_machine_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "use KVM in-kernel irqchip",
         }, {
-            .name = "kvm_shadow_mem",
+            .name = "kvm-shadow-mem",
+            .alias= "kvm_shadow_mem",
             .type = QEMU_OPT_SIZE,
             .help = "KVM shadow MMU size",
         }, {
diff --git a/qemu-options.hx b/qemu-options.hx
index b149852..b95438f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -33,7 +33,7 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                property accel=accel1[:accel2[:...]] selects accelerator\n"
     "                supported accelerators are kvm, xen, tcg (default: tcg)\n"
     "                kernel-irqchip=on|off controls accelerated irqchip support\n"
-    "                kvm_shadow_mem=size of KVM shadow MMU\n",
+    "                kvm-shadow-mem=size of KVM shadow MMU\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -machine [type=]@var{name}[,prop=@var{value}[,...]]
@@ -48,7 +48,7 @@ than one accelerator specified, the next one is used if the previous one fails
 to initialize.
 @item kernel-irqchip=on|off
 Enables in-kernel irqchip support for the chosen accelerator when available.
-@item kvm_shadow_mem=size
+@item kvm-shadow-mem=size
 Defines the size of the KVM shadow MMU.
 @end table
 ETEXI
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0d0d8f6..90bf4d4 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -698,7 +698,7 @@ int kvm_arch_init(KVMState *s)
 
     if (!QTAILQ_EMPTY(&list->head)) {
         shadow_mem = qemu_opt_get_size(QTAILQ_FIRST(&list->head),
-                                       "kvm_shadow_mem", -1);
+                                       "kvm-shadow-mem", -1);
         if (shadow_mem != -1) {
             shadow_mem /= 4096;
             ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 7/8] machine: rename phandle_start to phandle-start

[Luiz Capitulino]

QOM conversion wants option names with dashes. This commit does the
change for the phandle_start machine type option.

The old option name is still supported through an option alias for
backwards compatibility.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 device_tree.c | 2 +-
 qemu-config.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index b366fdd..fe59768 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -213,7 +213,7 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
         machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
         if (machine_opts) {
             const char *phandle_start;
-            phandle_start = qemu_opt_get(machine_opts, "phandle_start");
+            phandle_start = qemu_opt_get(machine_opts, "phandle-start");
             if (phandle_start) {
                 phandle = strtoul(phandle_start, NULL, 0);
             }
diff --git a/qemu-config.c b/qemu-config.c
index 909fae9..d888b5b 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -590,7 +590,8 @@ static QemuOptsList qemu_machine_opts = {
             .type = QEMU_OPT_STRING,
             .help = "Dump current dtb to a file and quit",
         }, {
-            .name = "phandle_start",
+            .name = "phandle-start",
+            .alias= "phandle_start",
             .type = QEMU_OPT_STRING,
             .help = "The first phandle ID we may generate dynamically",
         }, {
-- 
1.7.11.1.116.g8228a23

[Qemu-devel] [PATCH 8/8] machine: rename dt_compatible to dt-compatible

[Luiz Capitulino]

QOM conversion wants option names with dashes. This commit does the
change for the dt_compatible machine type option.

The old option name is still supported through an option alias for
backwards compatibility.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/ppce500_mpc8544ds.c | 2 +-
 qemu-config.c          | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index 8b9fd83..7b98011 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -148,7 +148,7 @@ static int mpc8544_load_device_tree(CPUPPCState *env,
         const char *tmp;
         dumpdtb = qemu_opt_get(machine_opts, "dumpdtb");
         dtb_file = qemu_opt_get(machine_opts, "dtb");
-        tmp = qemu_opt_get(machine_opts, "dt_compatible");
+        tmp = qemu_opt_get(machine_opts, "dt-compatible");
         if (tmp) {
             compatible = tmp;
             compatible_len = strlen(compatible) + 1;
diff --git a/qemu-config.c b/qemu-config.c
index d888b5b..9dac3be 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -595,7 +595,8 @@ static QemuOptsList qemu_machine_opts = {
             .type = QEMU_OPT_STRING,
             .help = "The first phandle ID we may generate dynamically",
         }, {
-            .name = "dt_compatible",
+            .name = "dt-compatible",
+            .alias= "dt_compatible",
             .type = QEMU_OPT_STRING,
             .help = "Overrides the \"compatible\" property of the dt root node",
         },
-- 
1.7.11.1.116.g8228a23

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Call qemu_opt_set() instead of duplicating opt_set().
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-option.c | 28 +---------------------------
>  1 file changed, 1 insertion(+), 27 deletions(-)
>
> diff --git a/qemu-option.c b/qemu-option.c
> index bb3886c..2cb2835 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> *name, const char *value,
>  
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>  {
> -    QemuOpt *opt;
> -    const QemuOptDesc *desc = opts->list->desc;
> -    int i;
> -
> -    for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> -            break;
> -        }
> -    }
> -    if (desc[i].name == NULL) {
> -        if (i == 0) {
> -            /* empty list -> allow any */;
> -        } else {
> -            qerror_report(QERR_INVALID_PARAMETER, name);
> -            return -1;
> -        }
> -    }
> -
> -    opt = g_malloc0(sizeof(*opt));
> -    opt->name = g_strdup(name);
> -    opt->opts = opts;
> -    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> -    if (desc[i].name != NULL) {
> -        opt->desc = desc+i;
> -    }
> -    opt->value.boolean = !!val;
> -    return 0;
> +    return qemu_opt_set(opts, name, val ? "on" : "off");
>  }
>  
>  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,

Does a bit more than just obvious code de-duplication.  Two things in
particular:

* Error reporting

  Old: qerror_report(); return -1

  New: opt_set() and qemu_opt_set() cooperate, like this:
       opt_set(): error_set(); return;
       qemu_opt_set():
            if (error_is_set(&local_err)) {
                qerror_report_err(local_err);
                error_free(local_err);
                return -1;
            }

  I guess the net effect is the same.  Not sure it's worth mentioning in
  the commit message.

* New sets opt->str to either "on" or "off" depending on val, then lets
  reconstructs the value with qemu_opt_parse().  Which can't fail then.
  I figure the latter part has no further impact.  But what about
  setting opt->str?  Is this a bug fix?

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Allow for specifying an alias for each option name, see next commits
> for examples.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  qemu-option.c | 5 +++--
>  qemu-option.h | 1 +
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-option.c b/qemu-option.c
> index 65ba1cf..b2f9e21 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>      int i;
>  
>      for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> +        if (strcmp(desc[i].name, name) == 0 ||
> +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>              return &desc[i];
>          }
>      }
> @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>      }
>  
>      opt = g_malloc0(sizeof(*opt));
> -    opt->name = g_strdup(name);
> +    opt->name = g_strdup(desc ? desc->name : name);
>      opt->opts = opts;
>      if (prepend) {
>          QTAILQ_INSERT_HEAD(&opts->head, opt, next);

Are you sure this hunk belongs to this patch?  If yes, please explain
why :)

> diff --git a/qemu-option.h b/qemu-option.h
> index 951dec3..7106d2f 100644
> --- a/qemu-option.h
> +++ b/qemu-option.h
> @@ -94,6 +94,7 @@ enum QemuOptType {
>  
>  typedef struct QemuOptDesc {
>      const char *name;
> +    const char *alias;
>      enum QemuOptType type;
>      const char *help;
>  } QemuOptDesc;

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Luiz Capitulino]

On Sat, 21 Jul 2012 10:09:09 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Call qemu_opt_set() instead of duplicating opt_set().
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-option.c | 28 +---------------------------
> >  1 file changed, 1 insertion(+), 27 deletions(-)
> >
> > diff --git a/qemu-option.c b/qemu-option.c
> > index bb3886c..2cb2835 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> > *name, const char *value,
> >  
> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> >  {
> > -    QemuOpt *opt;
> > -    const QemuOptDesc *desc = opts->list->desc;
> > -    int i;
> > -
> > -    for (i = 0; desc[i].name != NULL; i++) {
> > -        if (strcmp(desc[i].name, name) == 0) {
> > -            break;
> > -        }
> > -    }
> > -    if (desc[i].name == NULL) {
> > -        if (i == 0) {
> > -            /* empty list -> allow any */;
> > -        } else {
> > -            qerror_report(QERR_INVALID_PARAMETER, name);
> > -            return -1;
> > -        }
> > -    }
> > -
> > -    opt = g_malloc0(sizeof(*opt));
> > -    opt->name = g_strdup(name);
> > -    opt->opts = opts;
> > -    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> > -    if (desc[i].name != NULL) {
> > -        opt->desc = desc+i;
> > -    }
> > -    opt->value.boolean = !!val;
> > -    return 0;
> > +    return qemu_opt_set(opts, name, val ? "on" : "off");
> >  }
> >  
> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> 
> Does a bit more than just obvious code de-duplication.  Two things in
> particular:
> 
> * Error reporting
> 
>   Old: qerror_report(); return -1
> 
>   New: opt_set() and qemu_opt_set() cooperate, like this:
>        opt_set(): error_set(); return;
>        qemu_opt_set():
>             if (error_is_set(&local_err)) {
>                 qerror_report_err(local_err);
>                 error_free(local_err);
>                 return -1;
>             }
> 
>   I guess the net effect is the same.  Not sure it's worth mentioning in
>   the commit message.

The end result of calling qemu_opt_set_bool() is the same. The difference
between qerror_report() and qerror_report_err() is that the former gets error
information from the error call, while the latter gets error information
from the Error object. But they do the same thing.

> * New sets opt->str to either "on" or "off" depending on val, then lets
>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
>   I figure the latter part has no further impact.  But what about
>   setting opt->str?  Is this a bug fix?

I don't remember if opt->str is read after the QemuOpt object is built. If
it's, then yes, this is a bug fix. Otherwise it just make the final
QemuOpt object more 'conforming'.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Sat, 21 Jul 2012 10:11:39 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Allow for specifying an alias for each option name, see next commits
> > for examples.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  qemu-option.c | 5 +++--
> >  qemu-option.h | 1 +
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/qemu-option.c b/qemu-option.c
> > index 65ba1cf..b2f9e21 100644
> > --- a/qemu-option.c
> > +++ b/qemu-option.c
> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> >      int i;
> >  
> >      for (i = 0; desc[i].name != NULL; i++) {
> > -        if (strcmp(desc[i].name, name) == 0) {
> > +        if (strcmp(desc[i].name, name) == 0 ||
> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >              return &desc[i];
> >          }
> >      }
> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> >      }
> >  
> >      opt = g_malloc0(sizeof(*opt));
> > -    opt->name = g_strdup(name);
> > +    opt->name = g_strdup(desc ? desc->name : name);
> >      opt->opts = opts;
> >      if (prepend) {
> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> 
> Are you sure this hunk belongs to this patch?  If yes, please explain
> why :)

Yes, I think it's fine because the change that makes it necessary to choose
between desc->name and name is introduced by the hunk above.

Of course that it's possible to move this to a separate patch, but I don't
think it's worth it, as name is always valid with the current code.

> 
> > diff --git a/qemu-option.h b/qemu-option.h
> > index 951dec3..7106d2f 100644
> > --- a/qemu-option.h
> > +++ b/qemu-option.h
> > @@ -94,6 +94,7 @@ enum QemuOptType {
> >  
> >  typedef struct QemuOptDesc {
> >      const char *name;
> > +    const char *alias;
> >      enum QemuOptType type;
> >      const char *help;
> >  } QemuOptDesc;
> 

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 21 Jul 2012 10:09:09 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Call qemu_opt_set() instead of duplicating opt_set().
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  qemu-option.c | 28 +---------------------------
>> >  1 file changed, 1 insertion(+), 27 deletions(-)
>> >
>> > diff --git a/qemu-option.c b/qemu-option.c
>> > index bb3886c..2cb2835 100644
>> > --- a/qemu-option.c
>> > +++ b/qemu-option.c
>> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
>> > *name, const char *value,
>> >  
>> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>> >  {
>> > -    QemuOpt *opt;
>> > -    const QemuOptDesc *desc = opts->list->desc;
>> > -    int i;
>> > -
>> > -    for (i = 0; desc[i].name != NULL; i++) {
>> > -        if (strcmp(desc[i].name, name) == 0) {
>> > -            break;
>> > -        }
>> > -    }
>> > -    if (desc[i].name == NULL) {
>> > -        if (i == 0) {
>> > -            /* empty list -> allow any */;
>> > -        } else {
>> > -            qerror_report(QERR_INVALID_PARAMETER, name);
>> > -            return -1;
>> > -        }
>> > -    }
>> > -
>> > -    opt = g_malloc0(sizeof(*opt));
>> > -    opt->name = g_strdup(name);
>> > -    opt->opts = opts;
>> > -    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
>> > -    if (desc[i].name != NULL) {
>> > -        opt->desc = desc+i;
>> > -    }
>> > -    opt->value.boolean = !!val;
>> > -    return 0;
>> > +    return qemu_opt_set(opts, name, val ? "on" : "off");
>> >  }
>> >  
>> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>> 
>> Does a bit more than just obvious code de-duplication.  Two things in
>> particular:
>> 
>> * Error reporting
>> 
>>   Old: qerror_report(); return -1
>> 
>>   New: opt_set() and qemu_opt_set() cooperate, like this:
>>        opt_set(): error_set(); return;
>>        qemu_opt_set():
>>             if (error_is_set(&local_err)) {
>>                 qerror_report_err(local_err);
>>                 error_free(local_err);
>>                 return -1;
>>             }
>> 
>>   I guess the net effect is the same.  Not sure it's worth mentioning in
>>   the commit message.
>
> The end result of calling qemu_opt_set_bool() is the same. The difference
> between qerror_report() and qerror_report_err() is that the former gets error
> information from the error call, while the latter gets error information
> from the Error object. But they do the same thing.
>
>> * New sets opt->str to either "on" or "off" depending on val, then lets
>>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
>>   I figure the latter part has no further impact.  But what about
>>   setting opt->str?  Is this a bug fix?
>
> I don't remember if opt->str is read after the QemuOpt object is built. If
> it's, then yes, this is a bug fix. Otherwise it just make the final
> QemuOpt object more 'conforming'.

Uses of opt->str, and what happens when it isn't set:

* qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
  when value isn't the default value.

* qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
  like "on".  Wrong if the value is actually false.  Bug can bite when
  qemu_opts_validate() runs after qemu_opt_set_bool().

* qemu_opt_del(): passes NULL to g_free(), which is just fine.

* qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
  be prepared for it.

* qemu_opts_print(): prints NULL, which crashes on some systems.

* qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
  crashes.

Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
Can't see how to break it, but I didn't look hard.

I recommend to document the bug fix in the commit message.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 21 Jul 2012 10:11:39 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Allow for specifying an alias for each option name, see next commits
>> > for examples.
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  qemu-option.c | 5 +++--
>> >  qemu-option.h | 1 +
>> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/qemu-option.c b/qemu-option.c
>> > index 65ba1cf..b2f9e21 100644
>> > --- a/qemu-option.c
>> > +++ b/qemu-option.c
>> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>> >      int i;
>> >  
>> >      for (i = 0; desc[i].name != NULL; i++) {
>> > -        if (strcmp(desc[i].name, name) == 0) {
>> > +        if (strcmp(desc[i].name, name) == 0 ||
>> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>> >              return &desc[i];
>> >          }
>> >      }
>> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,

      static void opt_set(QemuOpts *opts, const char *name, const 
                          bool prepend, Error **errp)
      {
          QemuOpt *opt;
          const QemuOptDesc *desc;
          Error *local_err = NULL;

          desc = find_desc_by_name(opts->list->desc, name);
          if (!desc && !opts_accepts_any(opts)) {
              error_set(errp, QERR_INVALID_PARAMETER, name);
              return;
>> >      }
>> >  
>> >      opt = g_malloc0(sizeof(*opt));
>> > -    opt->name = g_strdup(name);
>> > +    opt->name = g_strdup(desc ? desc->name : name);
>> >      opt->opts = opts;
>> >      if (prepend) {
>> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
>> 
>> Are you sure this hunk belongs to this patch?  If yes, please explain
>> why :)
>
> Yes, I think it's fine because the change that makes it necessary to choose
> between desc->name and name is introduced by the hunk above.
>

I think I now get why you have this hunk:

We reach it only if the parameter with this name exists (desc non-null),
or opts accepts anthing (opts_accepts_any(opts).

If it exists, name equals desc->name before your patch.  But afterwards,
it could be either desc->name or desc->alias.  You normalize to
desc->name.

Else, all we can do is stick to name.

Correct?

If yes, then "normal" options and "accept any" options behave
differently: the former set opts->name to the canonical name, even when
the user uses an alias.  The latter set opts->name to whatever the user
uses.  I got a bad feeling about that.

> Of course that it's possible to move this to a separate patch, but I don't
> think it's worth it, as name is always valid with the current code.
>
>> > diff --git a/qemu-option.h b/qemu-option.h
>> > index 951dec3..7106d2f 100644
>> > --- a/qemu-option.h
>> > +++ b/qemu-option.h
>> > @@ -94,6 +94,7 @@ enum QemuOptType {
>> >  
>> >  typedef struct QemuOptDesc {
>> >      const char *name;
>> > +    const char *alias;
>> >      enum QemuOptType type;
>> >      const char *help;
>> >  } QemuOptDesc;
>> 

Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Luiz Capitulino]

On Mon, 23 Jul 2012 20:14:31 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Sat, 21 Jul 2012 10:09:09 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > Call qemu_opt_set() instead of duplicating opt_set().
> >> >
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > ---
> >> >  qemu-option.c | 28 +---------------------------
> >> >  1 file changed, 1 insertion(+), 27 deletions(-)
> >> >
> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> > index bb3886c..2cb2835 100644
> >> > --- a/qemu-option.c
> >> > +++ b/qemu-option.c
> >> > @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> >> > *name, const char *value,
> >> >  
> >> >  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
> >> >  {
> >> > -    QemuOpt *opt;
> >> > -    const QemuOptDesc *desc = opts->list->desc;
> >> > -    int i;
> >> > -
> >> > -    for (i = 0; desc[i].name != NULL; i++) {
> >> > -        if (strcmp(desc[i].name, name) == 0) {
> >> > -            break;
> >> > -        }
> >> > -    }
> >> > -    if (desc[i].name == NULL) {
> >> > -        if (i == 0) {
> >> > -            /* empty list -> allow any */;
> >> > -        } else {
> >> > -            qerror_report(QERR_INVALID_PARAMETER, name);
> >> > -            return -1;
> >> > -        }
> >> > -    }
> >> > -
> >> > -    opt = g_malloc0(sizeof(*opt));
> >> > -    opt->name = g_strdup(name);
> >> > -    opt->opts = opts;
> >> > -    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> >> > -    if (desc[i].name != NULL) {
> >> > -        opt->desc = desc+i;
> >> > -    }
> >> > -    opt->value.boolean = !!val;
> >> > -    return 0;
> >> > +    return qemu_opt_set(opts, name, val ? "on" : "off");
> >> >  }
> >> >  
> >> >  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
> >> 
> >> Does a bit more than just obvious code de-duplication.  Two things in
> >> particular:
> >> 
> >> * Error reporting
> >> 
> >>   Old: qerror_report(); return -1
> >> 
> >>   New: opt_set() and qemu_opt_set() cooperate, like this:
> >>        opt_set(): error_set(); return;
> >>        qemu_opt_set():
> >>             if (error_is_set(&local_err)) {
> >>                 qerror_report_err(local_err);
> >>                 error_free(local_err);
> >>                 return -1;
> >>             }
> >> 
> >>   I guess the net effect is the same.  Not sure it's worth mentioning in
> >>   the commit message.
> >
> > The end result of calling qemu_opt_set_bool() is the same. The difference
> > between qerror_report() and qerror_report_err() is that the former gets error
> > information from the error call, while the latter gets error information
> > from the Error object. But they do the same thing.
> >
> >> * New sets opt->str to either "on" or "off" depending on val, then lets
> >>   reconstructs the value with qemu_opt_parse().  Which can't fail then.
> >>   I figure the latter part has no further impact.  But what about
> >>   setting opt->str?  Is this a bug fix?
> >
> > I don't remember if opt->str is read after the QemuOpt object is built. If
> > it's, then yes, this is a bug fix. Otherwise it just make the final
> > QemuOpt object more 'conforming'.
> 
> Uses of opt->str, and what happens when it isn't set:
> 
> * qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
>   when value isn't the default value.
> 
> * qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
>   like "on".  Wrong if the value is actually false.  Bug can bite when
>   qemu_opts_validate() runs after qemu_opt_set_bool().
> 
> * qemu_opt_del(): passes NULL to g_free(), which is just fine.
> 
> * qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
>   be prepared for it.
> 
> * qemu_opts_print(): prints NULL, which crashes on some systems.
> 
> * qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
>   crashes.

Thanks for the clarification.

> 
> Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
> Can't see how to break it, but I didn't look hard.
> 
> I recommend to document the bug fix in the commit message.

Ok, will do.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Mon, 23 Jul 2012 20:33:52 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Sat, 21 Jul 2012 10:11:39 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > Allow for specifying an alias for each option name, see next commits
> >> > for examples.
> >> >
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> > ---
> >> >  qemu-option.c | 5 +++--
> >> >  qemu-option.h | 1 +
> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> > index 65ba1cf..b2f9e21 100644
> >> > --- a/qemu-option.c
> >> > +++ b/qemu-option.c
> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> >> >      int i;
> >> >  
> >> >      for (i = 0; desc[i].name != NULL; i++) {
> >> > -        if (strcmp(desc[i].name, name) == 0) {
> >> > +        if (strcmp(desc[i].name, name) == 0 ||
> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >> >              return &desc[i];
> >> >          }
> >> >      }
> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> 
>       static void opt_set(QemuOpts *opts, const char *name, const 
>                           bool prepend, Error **errp)
>       {
>           QemuOpt *opt;
>           const QemuOptDesc *desc;
>           Error *local_err = NULL;
> 
>           desc = find_desc_by_name(opts->list->desc, name);
>           if (!desc && !opts_accepts_any(opts)) {
>               error_set(errp, QERR_INVALID_PARAMETER, name);
>               return;
> >> >      }
> >> >  
> >> >      opt = g_malloc0(sizeof(*opt));
> >> > -    opt->name = g_strdup(name);
> >> > +    opt->name = g_strdup(desc ? desc->name : name);
> >> >      opt->opts = opts;
> >> >      if (prepend) {
> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> >> 
> >> Are you sure this hunk belongs to this patch?  If yes, please explain
> >> why :)
> >
> > Yes, I think it's fine because the change that makes it necessary to choose
> > between desc->name and name is introduced by the hunk above.
> >
> 
> I think I now get why you have this hunk:
> 
> We reach it only if the parameter with this name exists (desc non-null),
> or opts accepts anthing (opts_accepts_any(opts).
> 
> If it exists, name equals desc->name before your patch.  But afterwards,
> it could be either desc->name or desc->alias.  You normalize to
> desc->name.
> 
> Else, all we can do is stick to name.
> 
> Correct?

Yes.

> If yes, then "normal" options and "accept any" options behave
> differently: the former set opts->name to the canonical name, even when
> the user uses an alias.  The latter set opts->name to whatever the user
> uses.  I got a bad feeling about that.

Why? Or, more importantly, how do you think we should do it?

For 'normal' options, the alias is just a different name to refer to the
option name. At least in theory, it shouldn't matter that the option was
set through the alias.

For 'accept any' options, it's up to the code handling the options know
what to do with it. It can also support aliases if it wants to, or just
refuse it.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 23 Jul 2012 20:33:52 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Sat, 21 Jul 2012 10:11:39 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > Allow for specifying an alias for each option name, see next commits
>> >> > for examples.
>> >> >
>> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> > ---
>> >> >  qemu-option.c | 5 +++--
>> >> >  qemu-option.h | 1 +
>> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/qemu-option.c b/qemu-option.c
>> >> > index 65ba1cf..b2f9e21 100644
>> >> > --- a/qemu-option.c
>> >> > +++ b/qemu-option.c
>> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>> >> >      int i;
>> >> >  
>> >> >      for (i = 0; desc[i].name != NULL; i++) {
>> >> > -        if (strcmp(desc[i].name, name) == 0) {
>> >> > +        if (strcmp(desc[i].name, name) == 0 ||
>> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>> >> >              return &desc[i];
>> >> >          }
>> >> >      }
>> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>> 
>>       static void opt_set(QemuOpts *opts, const char *name, const 
>>                           bool prepend, Error **errp)
>>       {
>>           QemuOpt *opt;
>>           const QemuOptDesc *desc;
>>           Error *local_err = NULL;
>> 
>>           desc = find_desc_by_name(opts->list->desc, name);
>>           if (!desc && !opts_accepts_any(opts)) {
>>               error_set(errp, QERR_INVALID_PARAMETER, name);
>>               return;
>> >> >      }
>> >> >  
>> >> >      opt = g_malloc0(sizeof(*opt));
>> >> > -    opt->name = g_strdup(name);
>> >> > +    opt->name = g_strdup(desc ? desc->name : name);
>> >> >      opt->opts = opts;
>> >> >      if (prepend) {
>> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
>> >> 
>> >> Are you sure this hunk belongs to this patch?  If yes, please explain
>> >> why :)
>> >
>> > Yes, I think it's fine because the change that makes it necessary to choose
>> > between desc->name and name is introduced by the hunk above.
>> >
>> 
>> I think I now get why you have this hunk:
>> 
>> We reach it only if the parameter with this name exists (desc non-null),
>> or opts accepts anthing (opts_accepts_any(opts).
>> 
>> If it exists, name equals desc->name before your patch.  But afterwards,
>> it could be either desc->name or desc->alias.  You normalize to
>> desc->name.
>> 
>> Else, all we can do is stick to name.
>> 
>> Correct?
>
> Yes.
>
>> If yes, then "normal" options and "accept any" options behave
>> differently: the former set opts->name to the canonical name, even when
>> the user uses an alias.  The latter set opts->name to whatever the user
>> uses.  I got a bad feeling about that.
>
> Why? Or, more importantly, how do you think we should do it?
>
> For 'normal' options, the alias is just a different name to refer to the
> option name. At least in theory, it shouldn't matter that the option was
> set through the alias.
>
> For 'accept any' options, it's up to the code handling the options know
> what to do with it. It can also support aliases if it wants to, or just
> refuse it.

Let's examine how opt->name is used.

* qemu_opt_find(): helper for the qemu_opt_get*() functions, compares
  opt->name to argument name.  opt->name must be a canonical name for
  that to work.

* qemu_opt_parse(): passes opt->name to parse_option_*() functions,
  which use it for error reporting.  opt->name should be whatever the
  user used, or else the error messages will confusing.

* qemu_opt_del(): passes it to g_free().

* qemu_opt_foreach(): passes it to the callback.  Whether the callback
  expects the canonical name or what the user used is unclear.  Current
  callbacks:

  - config_write_opt(): either works.  With the canonical name,
    -writeconfig canconicalizes option parameters.  With the user's
    name, it sticks to what the user used.

  - set_property(): compares it to qdev property names.  Needs canonical
    name.

  - net_init_slirp_configs(): compares it to string literals.  Needs
    canonical name.

  - cpudef_setfield(): compares it to string literals, and puts it into
    error messages.  The former needs the canonical name, the latter
    user's name.  Fun.

  - add_channel(): compares it to string literals.  Needs canonical
    name.

* qemu_opts_print(): unused.  Whether to print canonical name or user's
  name is unclear.

* qemu_opts_to_qdict(): only used for monitor argument type 'O'.  Needs
  canonical name.

* qemu_opts_validate(): expects user's names.

I think we need to define the meaning of opt->name.  We might have to
provide both names.

Your patch sets it to the canonical name unless desc is empty.  Aliases
break qemu_opt_parse() error reporting.  Looks fixable, e.g. set
opt->name to the user's name first, change it to the canonical name
after qemu_opt_parse().

Your patch sets it to the user's name if desc is empty, i.e. for
qemu_device_opts, qemu_netdev_opts, qemu_net_opts.

qemu_device_opts is handed off to set_property().  Bypasses you alias
code completely, thus no problem.

The other two get passed to qemu_opts_validate().  Since
qemu_opts_validate() doesn't change opts->name, the error messages from
qemu_opt_parse() are fine here.  But aliases break the later
qemu_opt_get() calls.  Fixable the same way: change opt->name to the
canonical name after qemu_opt_parse().

Instead of overloading opt->name to mean user's name before parse and
canonical name afterwards, you may want to provide separate QemuOpts
members.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Tue, 24 Jul 2012 11:19:45 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Mon, 23 Jul 2012 20:33:52 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Sat, 21 Jul 2012 10:11:39 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > Allow for specifying an alias for each option name, see next commits
> >> >> > for examples.
> >> >> >
> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> >> > ---
> >> >> >  qemu-option.c | 5 +++--
> >> >> >  qemu-option.h | 1 +
> >> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> >> > index 65ba1cf..b2f9e21 100644
> >> >> > --- a/qemu-option.c
> >> >> > +++ b/qemu-option.c
> >> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> >> >> >      int i;
> >> >> >  
> >> >> >      for (i = 0; desc[i].name != NULL; i++) {
> >> >> > -        if (strcmp(desc[i].name, name) == 0) {
> >> >> > +        if (strcmp(desc[i].name, name) == 0 ||
> >> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >> >> >              return &desc[i];
> >> >> >          }
> >> >> >      }
> >> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> >> 
> >>       static void opt_set(QemuOpts *opts, const char *name, const 
> >>                           bool prepend, Error **errp)
> >>       {
> >>           QemuOpt *opt;
> >>           const QemuOptDesc *desc;
> >>           Error *local_err = NULL;
> >> 
> >>           desc = find_desc_by_name(opts->list->desc, name);
> >>           if (!desc && !opts_accepts_any(opts)) {
> >>               error_set(errp, QERR_INVALID_PARAMETER, name);
> >>               return;
> >> >> >      }
> >> >> >  
> >> >> >      opt = g_malloc0(sizeof(*opt));
> >> >> > -    opt->name = g_strdup(name);
> >> >> > +    opt->name = g_strdup(desc ? desc->name : name);
> >> >> >      opt->opts = opts;
> >> >> >      if (prepend) {
> >> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> >> >> 
> >> >> Are you sure this hunk belongs to this patch?  If yes, please explain
> >> >> why :)
> >> >
> >> > Yes, I think it's fine because the change that makes it necessary to choose
> >> > between desc->name and name is introduced by the hunk above.
> >> >
> >> 
> >> I think I now get why you have this hunk:
> >> 
> >> We reach it only if the parameter with this name exists (desc non-null),
> >> or opts accepts anthing (opts_accepts_any(opts).
> >> 
> >> If it exists, name equals desc->name before your patch.  But afterwards,
> >> it could be either desc->name or desc->alias.  You normalize to
> >> desc->name.
> >> 
> >> Else, all we can do is stick to name.
> >> 
> >> Correct?
> >
> > Yes.
> >
> >> If yes, then "normal" options and "accept any" options behave
> >> differently: the former set opts->name to the canonical name, even when
> >> the user uses an alias.  The latter set opts->name to whatever the user
> >> uses.  I got a bad feeling about that.
> >
> > Why? Or, more importantly, how do you think we should do it?
> >
> > For 'normal' options, the alias is just a different name to refer to the
> > option name. At least in theory, it shouldn't matter that the option was
> > set through the alias.
> >
> > For 'accept any' options, it's up to the code handling the options know
> > what to do with it. It can also support aliases if it wants to, or just
> > refuse it.
> 
> Let's examine how opt->name is used.
> 
> * qemu_opt_find(): helper for the qemu_opt_get*() functions, compares
>   opt->name to argument name.  opt->name must be a canonical name for
>   that to work.
> 
> * qemu_opt_parse(): passes opt->name to parse_option_*() functions,
>   which use it for error reporting.  opt->name should be whatever the
>   user used, or else the error messages will confusing.
> 
> * qemu_opt_del(): passes it to g_free().
> 
> * qemu_opt_foreach(): passes it to the callback.  Whether the callback
>   expects the canonical name or what the user used is unclear.  Current
>   callbacks:
> 
>   - config_write_opt(): either works.  With the canonical name,
>     -writeconfig canconicalizes option parameters.  With the user's
>     name, it sticks to what the user used.
> 
>   - set_property(): compares it to qdev property names.  Needs canonical
>     name.
> 
>   - net_init_slirp_configs(): compares it to string literals.  Needs
>     canonical name.
> 
>   - cpudef_setfield(): compares it to string literals, and puts it into
>     error messages.  The former needs the canonical name, the latter
>     user's name.  Fun.
> 
>   - add_channel(): compares it to string literals.  Needs canonical
>     name.
> 
> * qemu_opts_print(): unused.  Whether to print canonical name or user's
>   name is unclear.
> 
> * qemu_opts_to_qdict(): only used for monitor argument type 'O'.  Needs
>   canonical name.
> 
> * qemu_opts_validate(): expects user's names.
> 
> I think we need to define the meaning of opt->name.  We might have to
> provide both names.
> 
> Your patch sets it to the canonical name unless desc is empty.  Aliases
> break qemu_opt_parse() error reporting.  Looks fixable, e.g. set
> opt->name to the user's name first, change it to the canonical name
> after qemu_opt_parse().

I'd prefer to store the alias and have a function like
qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
that either, returns the canonical name if the alias is not set, or the alias
if it's set.

Then any function that needs to know what the user passed can call
qemu_opt_get_passed_name().

What do you think?

> 
> Your patch sets it to the user's name if desc is empty, i.e. for
> qemu_device_opts, qemu_netdev_opts, qemu_net_opts.
> 
> qemu_device_opts is handed off to set_property().  Bypasses you alias
> code completely, thus no problem.
> 
> The other two get passed to qemu_opts_validate().  Since
> qemu_opts_validate() doesn't change opts->name, the error messages from
> qemu_opt_parse() are fine here.  But aliases break the later
> qemu_opt_get() calls.  Fixable the same way: change opt->name to the
> canonical name after qemu_opt_parse().

Not sure I follow, how does this break qemu_opt_get() calls?

> 
> Instead of overloading opt->name to mean user's name before parse and
> canonical name afterwards, you may want to provide separate QemuOpts
> members.

Yes, I can do that, but note that the hunk that generated this discussion
will stay the same in the meaning that opt->name will store the canonical
name (even if the alias was passed). If any option is to be accepted,
name will store the passed option.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 24 Jul 2012 11:19:45 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Mon, 23 Jul 2012 20:33:52 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Sat, 21 Jul 2012 10:11:39 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > Allow for specifying an alias for each option name, see next commits
>> >> >> > for examples.
>> >> >> >
>> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> >> >> > ---
>> >> >> >  qemu-option.c | 5 +++--
>> >> >> >  qemu-option.h | 1 +
>> >> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >> >> >
>> >> >> > diff --git a/qemu-option.c b/qemu-option.c
>> >> >> > index 65ba1cf..b2f9e21 100644
>> >> >> > --- a/qemu-option.c
>> >> >> > +++ b/qemu-option.c
>> >> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
>> >> >> >      int i;
>> >> >> >  
>> >> >> >      for (i = 0; desc[i].name != NULL; i++) {
>> >> >> > -        if (strcmp(desc[i].name, name) == 0) {
>> >> >> > +        if (strcmp(desc[i].name, name) == 0 ||
>> >> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
>> >> >> >              return &desc[i];
>> >> >> >          }
>> >> >> >      }
>> >> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>> >> 
>> >>       static void opt_set(QemuOpts *opts, const char *name, const 
>> >>                           bool prepend, Error **errp)
>> >>       {
>> >>           QemuOpt *opt;
>> >>           const QemuOptDesc *desc;
>> >>           Error *local_err = NULL;
>> >> 
>> >>           desc = find_desc_by_name(opts->list->desc, name);
>> >>           if (!desc && !opts_accepts_any(opts)) {
>> >>               error_set(errp, QERR_INVALID_PARAMETER, name);
>> >>               return;
>> >> >> >      }
>> >> >> >  
>> >> >> >      opt = g_malloc0(sizeof(*opt));
>> >> >> > -    opt->name = g_strdup(name);
>> >> >> > +    opt->name = g_strdup(desc ? desc->name : name);
>> >> >> >      opt->opts = opts;
>> >> >> >      if (prepend) {
>> >> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
>> >> >> 
>> >> >> Are you sure this hunk belongs to this patch?  If yes, please explain
>> >> >> why :)
>> >> >
>> >> > Yes, I think it's fine because the change that makes it
>> >> > necessary to choose
>> >> > between desc->name and name is introduced by the hunk above.
>> >> >
>> >> 
>> >> I think I now get why you have this hunk:
>> >> 
>> >> We reach it only if the parameter with this name exists (desc non-null),
>> >> or opts accepts anthing (opts_accepts_any(opts).
>> >> 
>> >> If it exists, name equals desc->name before your patch.  But afterwards,
>> >> it could be either desc->name or desc->alias.  You normalize to
>> >> desc->name.
>> >> 
>> >> Else, all we can do is stick to name.
>> >> 
>> >> Correct?
>> >
>> > Yes.
>> >
>> >> If yes, then "normal" options and "accept any" options behave
>> >> differently: the former set opts->name to the canonical name, even when
>> >> the user uses an alias.  The latter set opts->name to whatever the user
>> >> uses.  I got a bad feeling about that.
>> >
>> > Why? Or, more importantly, how do you think we should do it?
>> >
>> > For 'normal' options, the alias is just a different name to refer to the
>> > option name. At least in theory, it shouldn't matter that the option was
>> > set through the alias.
>> >
>> > For 'accept any' options, it's up to the code handling the options know
>> > what to do with it. It can also support aliases if it wants to, or just
>> > refuse it.
>> 
>> Let's examine how opt->name is used.
>> 
>> * qemu_opt_find(): helper for the qemu_opt_get*() functions, compares
>>   opt->name to argument name.  opt->name must be a canonical name for
>>   that to work.
>> 
>> * qemu_opt_parse(): passes opt->name to parse_option_*() functions,
>>   which use it for error reporting.  opt->name should be whatever the
>>   user used, or else the error messages will confusing.
>> 
>> * qemu_opt_del(): passes it to g_free().
>> 
>> * qemu_opt_foreach(): passes it to the callback.  Whether the callback
>>   expects the canonical name or what the user used is unclear.  Current
>>   callbacks:
>> 
>>   - config_write_opt(): either works.  With the canonical name,
>>     -writeconfig canconicalizes option parameters.  With the user's
>>     name, it sticks to what the user used.
>> 
>>   - set_property(): compares it to qdev property names.  Needs canonical
>>     name.
>> 
>>   - net_init_slirp_configs(): compares it to string literals.  Needs
>>     canonical name.
>> 
>>   - cpudef_setfield(): compares it to string literals, and puts it into
>>     error messages.  The former needs the canonical name, the latter
>>     user's name.  Fun.
>> 
>>   - add_channel(): compares it to string literals.  Needs canonical
>>     name.
>> 
>> * qemu_opts_print(): unused.  Whether to print canonical name or user's
>>   name is unclear.
>> 
>> * qemu_opts_to_qdict(): only used for monitor argument type 'O'.  Needs
>>   canonical name.
>> 
>> * qemu_opts_validate(): expects user's names.
>> 
>> I think we need to define the meaning of opt->name.  We might have to
>> provide both names.
>> 
>> Your patch sets it to the canonical name unless desc is empty.  Aliases
>> break qemu_opt_parse() error reporting.  Looks fixable, e.g. set
>> opt->name to the user's name first, change it to the canonical name
>> after qemu_opt_parse().
>
> I'd prefer to store the alias and have a function like
> qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
> that either, returns the canonical name if the alias is not set, or the alias
> if it's set.
>
> Then any function that needs to know what the user passed can call
> qemu_opt_get_passed_name().
>
> What do you think?

Parameters?  The QemuOpt, I guess.

Consider cpudef_setfield().  How do you plan to call
qemu_opt_get_passed_name() there?

>> 
>> Your patch sets it to the user's name if desc is empty, i.e. for
>> qemu_device_opts, qemu_netdev_opts, qemu_net_opts.
>> 
>> qemu_device_opts is handed off to set_property().  Bypasses you alias
>> code completely, thus no problem.
>> 
>> The other two get passed to qemu_opts_validate().  Since
>> qemu_opts_validate() doesn't change opts->name, the error messages from
>> qemu_opt_parse() are fine here.  But aliases break the later
>> qemu_opt_get() calls.  Fixable the same way: change opt->name to the
>> canonical name after qemu_opt_parse().
>
> Not sure I follow, how does this break qemu_opt_get() calls?

Say you add alias "address" to parameter "addr" in
net_client_type[NET_CLIENT_TYPE_NIC].  Yes, that's pointless; it's just
an example.

If the user uses the alias, your opt_set() sets opt->name to "address".
net_client_init() passes opts to qemu_opts_validate(), which recognizes
"address".  It then passes opts to net_init_nic() (indirectly through
net_client_types[NET_CLIENT_TYPE_NIC].init()).  net_init_nic() calls
qemu_opt_get(opts, "addr"), which fails, because opt->str is still
"address".

>> Instead of overloading opt->name to mean user's name before parse and
>> canonical name afterwards, you may want to provide separate QemuOpts
>> members.
>
> Yes, I can do that, but note that the hunk that generated this discussion
> will stay the same in the meaning that opt->name will store the canonical
> name (even if the alias was passed). If any option is to be accepted,
> name will store the passed option.

My gut suggests one member for the canonical name (null until it becomes
known), and another member for the user's name.

Re: [Qemu-devel] [PATCH 4/8] qemu-option: add alias support

[Luiz Capitulino]

On Tue, 24 Jul 2012 18:01:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Tue, 24 Jul 2012 11:19:45 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Mon, 23 Jul 2012 20:33:52 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Sat, 21 Jul 2012 10:11:39 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> 
> >> >> >> > Allow for specifying an alias for each option name, see next commits
> >> >> >> > for examples.
> >> >> >> >
> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> >> >> > ---
> >> >> >> >  qemu-option.c | 5 +++--
> >> >> >> >  qemu-option.h | 1 +
> >> >> >> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >> >> >> >
> >> >> >> > diff --git a/qemu-option.c b/qemu-option.c
> >> >> >> > index 65ba1cf..b2f9e21 100644
> >> >> >> > --- a/qemu-option.c
> >> >> >> > +++ b/qemu-option.c
> >> >> >> > @@ -623,7 +623,8 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> >> >> >> >      int i;
> >> >> >> >  
> >> >> >> >      for (i = 0; desc[i].name != NULL; i++) {
> >> >> >> > -        if (strcmp(desc[i].name, name) == 0) {
> >> >> >> > +        if (strcmp(desc[i].name, name) == 0 ||
> >> >> >> > +            (desc[i].alias && strcmp(desc[i].alias, name) == 0)) {
> >> >> >> >              return &desc[i];
> >> >> >> >          }
> >> >> >> >      }
> >> >> >> > @@ -645,7 +646,7 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
> >> >> 
> >> >>       static void opt_set(QemuOpts *opts, const char *name, const 
> >> >>                           bool prepend, Error **errp)
> >> >>       {
> >> >>           QemuOpt *opt;
> >> >>           const QemuOptDesc *desc;
> >> >>           Error *local_err = NULL;
> >> >> 
> >> >>           desc = find_desc_by_name(opts->list->desc, name);
> >> >>           if (!desc && !opts_accepts_any(opts)) {
> >> >>               error_set(errp, QERR_INVALID_PARAMETER, name);
> >> >>               return;
> >> >> >> >      }
> >> >> >> >  
> >> >> >> >      opt = g_malloc0(sizeof(*opt));
> >> >> >> > -    opt->name = g_strdup(name);
> >> >> >> > +    opt->name = g_strdup(desc ? desc->name : name);
> >> >> >> >      opt->opts = opts;
> >> >> >> >      if (prepend) {
> >> >> >> >          QTAILQ_INSERT_HEAD(&opts->head, opt, next);
> >> >> >> 
> >> >> >> Are you sure this hunk belongs to this patch?  If yes, please explain
> >> >> >> why :)
> >> >> >
> >> >> > Yes, I think it's fine because the change that makes it
> >> >> > necessary to choose
> >> >> > between desc->name and name is introduced by the hunk above.
> >> >> >
> >> >> 
> >> >> I think I now get why you have this hunk:
> >> >> 
> >> >> We reach it only if the parameter with this name exists (desc non-null),
> >> >> or opts accepts anthing (opts_accepts_any(opts).
> >> >> 
> >> >> If it exists, name equals desc->name before your patch.  But afterwards,
> >> >> it could be either desc->name or desc->alias.  You normalize to
> >> >> desc->name.
> >> >> 
> >> >> Else, all we can do is stick to name.
> >> >> 
> >> >> Correct?
> >> >
> >> > Yes.
> >> >
> >> >> If yes, then "normal" options and "accept any" options behave
> >> >> differently: the former set opts->name to the canonical name, even when
> >> >> the user uses an alias.  The latter set opts->name to whatever the user
> >> >> uses.  I got a bad feeling about that.
> >> >
> >> > Why? Or, more importantly, how do you think we should do it?
> >> >
> >> > For 'normal' options, the alias is just a different name to refer to the
> >> > option name. At least in theory, it shouldn't matter that the option was
> >> > set through the alias.
> >> >
> >> > For 'accept any' options, it's up to the code handling the options know
> >> > what to do with it. It can also support aliases if it wants to, or just
> >> > refuse it.
> >> 
> >> Let's examine how opt->name is used.
> >> 
> >> * qemu_opt_find(): helper for the qemu_opt_get*() functions, compares
> >>   opt->name to argument name.  opt->name must be a canonical name for
> >>   that to work.
> >> 
> >> * qemu_opt_parse(): passes opt->name to parse_option_*() functions,
> >>   which use it for error reporting.  opt->name should be whatever the
> >>   user used, or else the error messages will confusing.
> >> 
> >> * qemu_opt_del(): passes it to g_free().
> >> 
> >> * qemu_opt_foreach(): passes it to the callback.  Whether the callback
> >>   expects the canonical name or what the user used is unclear.  Current
> >>   callbacks:
> >> 
> >>   - config_write_opt(): either works.  With the canonical name,
> >>     -writeconfig canconicalizes option parameters.  With the user's
> >>     name, it sticks to what the user used.
> >> 
> >>   - set_property(): compares it to qdev property names.  Needs canonical
> >>     name.
> >> 
> >>   - net_init_slirp_configs(): compares it to string literals.  Needs
> >>     canonical name.
> >> 
> >>   - cpudef_setfield(): compares it to string literals, and puts it into
> >>     error messages.  The former needs the canonical name, the latter
> >>     user's name.  Fun.
> >> 
> >>   - add_channel(): compares it to string literals.  Needs canonical
> >>     name.
> >> 
> >> * qemu_opts_print(): unused.  Whether to print canonical name or user's
> >>   name is unclear.
> >> 
> >> * qemu_opts_to_qdict(): only used for monitor argument type 'O'.  Needs
> >>   canonical name.
> >> 
> >> * qemu_opts_validate(): expects user's names.
> >> 
> >> I think we need to define the meaning of opt->name.  We might have to
> >> provide both names.
> >> 
> >> Your patch sets it to the canonical name unless desc is empty.  Aliases
> >> break qemu_opt_parse() error reporting.  Looks fixable, e.g. set
> >> opt->name to the user's name first, change it to the canonical name
> >> after qemu_opt_parse().
> >
> > I'd prefer to store the alias and have a function like
> > qemu_opt_get_passed_name() (please, suggest a better name, I'm bad with that)
> > that either, returns the canonical name if the alias is not set, or the alias
> > if it's set.
> >
> > Then any function that needs to know what the user passed can call
> > qemu_opt_get_passed_name().
> >
> > What do you think?
> 
> Parameters?  The QemuOpt, I guess.

Yes.

> Consider cpudef_setfield().  How do you plan to call
> qemu_opt_get_passed_name() there?

Hmm, I expected qemu_opt_foreach() would pass opt to the callback function.
But that's not the case. It's doable to change all callbacks, but honestly,
the work in this series has gone beyond my original planning...

> 
> >> 
> >> Your patch sets it to the user's name if desc is empty, i.e. for
> >> qemu_device_opts, qemu_netdev_opts, qemu_net_opts.
> >> 
> >> qemu_device_opts is handed off to set_property().  Bypasses you alias
> >> code completely, thus no problem.
> >> 
> >> The other two get passed to qemu_opts_validate().  Since
> >> qemu_opts_validate() doesn't change opts->name, the error messages from
> >> qemu_opt_parse() are fine here.  But aliases break the later
> >> qemu_opt_get() calls.  Fixable the same way: change opt->name to the
> >> canonical name after qemu_opt_parse().
> >
> > Not sure I follow, how does this break qemu_opt_get() calls?
> 
> Say you add alias "address" to parameter "addr" in
> net_client_type[NET_CLIENT_TYPE_NIC].  Yes, that's pointless; it's just
> an example.
> 
> If the user uses the alias, your opt_set() sets opt->name to "address".

opt->name is never set to the alias, it's always the canonical name.

> net_client_init() passes opts to qemu_opts_validate(), which recognizes
> "address".  It then passes opts to net_init_nic() (indirectly through
> net_client_types[NET_CLIENT_TYPE_NIC].init()).  net_init_nic() calls
> qemu_opt_get(opts, "addr"), which fails, because opt->str is still
> "address".
> 
> >> Instead of overloading opt->name to mean user's name before parse and
> >> canonical name afterwards, you may want to provide separate QemuOpts
> >> members.
> >
> > Yes, I can do that, but note that the hunk that generated this discussion
> > will stay the same in the meaning that opt->name will store the canonical
> > name (even if the alias was passed). If any option is to be accepted,
> > name will store the passed option.
> 
> My gut suggests one member for the canonical name (null until it becomes
> known), and another member for the user's name.

Yes, that's probably the right way to do this.