[Qemu-devel] [PATCH v5 for 2.0 0/3] ABI change: change group name of option table to match with option name

[Qemu-devel] [PATCH v5 for 2.0 0/3] ABI change: change group name of option table to match with option name

[Amos Kong]

This patchset changes group names of option tables to match with option name,
this breakes ABI, release note was updated.

V4: fix tpmdev, add name matching test (markus)
V5: adjust patch order (paolo)

Amos Kong (3):
  only add qemu_tpmdev_opts when CONFIG_TPM is defined
  update names in option tables to match with actual command-line
    spelling
  abort QEMU if group name in option table doesn't match with defined
    option name

 hw/acpi/core.c        |  8 ++++----
 hw/nvram/fw_cfg.c     |  4 ++--
 include/qemu/option.h |  2 +-
 qemu-options.h        | 12 ++++++++++++
 util/qemu-config.c    | 28 ++++++++++++++++++++++++++++
 vl.c                  | 37 +++++++++++++------------------------
 6 files changed, 60 insertions(+), 31 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Amos Kong]

Signed-off-by: Amos Kong <akong@redhat.com>
---
 vl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/vl.c b/vl.c
index 2355227..596ecfa 100644
--- a/vl.c
+++ b/vl.c
@@ -449,6 +449,7 @@ static QemuOptsList qemu_object_opts = {
     },
 };
 
+#ifdef CONFIG_TPM
 static QemuOptsList qemu_tpmdev_opts = {
     .name = "tpmdev",
     .implied_opt_name = "type",
@@ -458,6 +459,7 @@ static QemuOptsList qemu_tpmdev_opts = {
         { /* end of list */ }
     },
 };
+#endif
 
 static QemuOptsList qemu_realtime_opts = {
     .name = "realtime",
@@ -2992,7 +2994,9 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_sandbox_opts);
     qemu_add_opts(&qemu_add_fd_opts);
     qemu_add_opts(&qemu_object_opts);
+#ifdef CONFIG_TPM
     qemu_add_opts(&qemu_tpmdev_opts);
+#endif
     qemu_add_opts(&qemu_realtime_opts);
     qemu_add_opts(&qemu_msg_opts);
     qemu_add_opts(&qemu_name_opts);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  vl.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/vl.c b/vl.c
> index 2355227..596ecfa 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -449,6 +449,7 @@ static QemuOptsList qemu_object_opts = {
>      },
>  };
>  
> +#ifdef CONFIG_TPM
>  static QemuOptsList qemu_tpmdev_opts = {
>      .name = "tpmdev",
>      .implied_opt_name = "type",
> @@ -458,6 +459,7 @@ static QemuOptsList qemu_tpmdev_opts = {
>          { /* end of list */ }
>      },
>  };
> +#endif
>  
>  static QemuOptsList qemu_realtime_opts = {
>      .name = "realtime",
> @@ -2992,7 +2994,9 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_sandbox_opts);
>      qemu_add_opts(&qemu_add_fd_opts);
>      qemu_add_opts(&qemu_object_opts);
> +#ifdef CONFIG_TPM
>      qemu_add_opts(&qemu_tpmdev_opts);
> +#endif
>      qemu_add_opts(&qemu_realtime_opts);
>      qemu_add_opts(&qemu_msg_opts);
>      qemu_add_opts(&qemu_name_opts);

Making this conditional permits checking whether TPM is enabled via
QMP.  Mentioning that in the commit message wouldn't hurt.

Precedence for conditional adding: "iscsi" (CONFIG_LIBISCSI), "fsdev"
(CONFIG_VIRTFS), and possibly more.  But it's done differently there: we
call qemu_add_opts() from block/iscsi.c, fsdev/qemu-fsdev-opts.c.  Call
it from tpm.c?  Could be done as follow-up cleanup, if that helps
getting the fix into 2.0.

Related: "add-fd" is defined unconditionally, but works only #ifndef
_WIN32.  Should it be made conditional to permit querying via QMP?

Taking a step back: quite a few command line options make sense only in
certain build configurations.  We deal with that in several different
ways:

1. Target-specific options: qemu-options.hx declares a target mask.
   main() rejects options that don't apply to the target.

   Example: --no-acpi is only valid for QEMU_ARCH_I386.

2. Options specific to the host OS are recognized by
   os_parse_cmd_args().  Any of them not recognized by the host OS's
   os_parse_cmd_args() are silently ignored.  *boggle*

   Example: --runas is ignored by the Windows build.

3. Options depending on configuration are handled in (at least) three
   ways:

   a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
      silently ignored when FOO isn't enabled.  *boggle again*

      Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.

   b. The option is always recognized, but rejected when #ifndef
      CONFIG_FOO.

      Example: --curses is rejected #ifndef CONFIG_CURSES.

   c. The option is always recognized, but rejected when its
      QemuOptsList hasn't been registered.  Essentially just an #if-less
      way to do 3b.

      Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
      compiled in.

In my opinion, silently ignoring an option is a bug until proven
otherwise.

Whether a non-sensical option should be recognized and rejected, or just
not recognized is debatable.

Regardless, telling QMP clients that an option works when it's always
rejected isn't useful.  We can either hide them in QMP, or add suitable
"invalid" markers, possibly identifying the reason.  Let's hide, unless
somebody can come up with a convincing use case for the additional
information.

For each of the cases above, how can we hide?  

1. Easy, check the target mask.

2. Turning "makes sense for host OS" from code into data we can check.

3a. Likewise.

3b. Likewise.

3c. If qemu-options.hx declares the QemuOptsList name, we can check
    whether the named list exists.  Could also be used to factor the
    qemu_opt_parse() out of the option switch.

We may not be able to get this wrapped in time for 2.0.  I'm not opposed
to a partial solution in 2.0, but let's think through the full solution,
to ensure the partial solution doesn't conflict with it.

Re: [Qemu-devel] [libvirt] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

On 03/28/2014 06:04 AM, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 

> Taking a step back: quite a few command line options make sense only in
> certain build configurations.  We deal with that in several different
> ways:
> 
> 1. Target-specific options: qemu-options.hx declares a target mask.
>    main() rejects options that don't apply to the target.
> 
>    Example: --no-acpi is only valid for QEMU_ARCH_I386.

Which means 'query-command-line-options' should not report 'no-acpi'
except when built for i386 emulation.

> 
> 2. Options specific to the host OS are recognized by
>    os_parse_cmd_args().  Any of them not recognized by the host OS's
>    os_parse_cmd_args() are silently ignored.  *boggle*
> 
>    Example: --runas is ignored by the Windows build.

Sounds like a bug.  Libvirt doesn't yet run qemu on a Windows build, but
ideally, 'query-command-line-options' should not report 'runas' on a
qemu binary built for windows.

> 
> 3. Options depending on configuration are handled in (at least) three
>    ways:
> 
>    a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
>       silently ignored when FOO isn't enabled.  *boggle again*
> 
>       Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.

Eww.  That's a bug - advertising a feature only to silently ignore it is
not helpful.

> 
>    b. The option is always recognized, but rejected when #ifndef
>       CONFIG_FOO.
> 
>       Example: --curses is rejected #ifndef CONFIG_CURSES.

Recognizing and rejecting with a nice message, vs. not recognizing and
giving a generic 'unknown option' message, both have the same net
effect.  It's more code to give the nice message, and is helpful to
humans, but if the option is omitted from 'query-command-line-options',
it makes no difference to libvirt.

> 
>    c. The option is always recognized, but rejected when its
>       QemuOptsList hasn't been registered.  Essentially just an #if-less
>       way to do 3b.
> 
>       Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
>       compiled in.
> 
> In my opinion, silently ignoring an option is a bug until proven
> otherwise.

Agreed.

> 
> Whether a non-sensical option should be recognized and rejected, or just
> not recognized is debatable.

Less code to not recognize it, you are then up to the question of
whether the nicer error message warrants the extra code.

> 
> Regardless, telling QMP clients that an option works when it's always
> rejected isn't useful.  We can either hide them in QMP, or add suitable
> "invalid" markers, possibly identifying the reason.  Let's hide, unless
> somebody can come up with a convincing use case for the additional
> information.

Agreed - hiding is nicer than having to expose yet more QMP details to
mark an option that is "recognized but will never work".

> 
> For each of the cases above, how can we hide?  
> 
> 1. Easy, check the target mask.
> 
> 2. Turning "makes sense for host OS" from code into data we can check.
> 
> 3a. Likewise.
> 
> 3b. Likewise.
> 
> 3c. If qemu-options.hx declares the QemuOptsList name, we can check
>     whether the named list exists.  Could also be used to factor the
>     qemu_opt_parse() out of the option switch.
> 
> We may not be able to get this wrapped in time for 2.0.  I'm not opposed
> to a partial solution in 2.0, but let's think through the full solution,
> to ensure the partial solution doesn't conflict with it.

We're no worse than we were for 1.5 when query-command-line-options was
first introduced - at this point, since we're not fixing a regression
and since the bug is longstanding, it may be wiser to just leave 2.0
as-is and save all the work for 2.1, rather than rushing in a partial
fix for 2.0 only to have to redo it again later.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [libvirt] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/28/2014 06:04 AM, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>> 
>
>> Taking a step back: quite a few command line options make sense only in
>> certain build configurations.  We deal with that in several different
>> ways:
>> 
>> 1. Target-specific options: qemu-options.hx declares a target mask.
>>    main() rejects options that don't apply to the target.
>> 
>>    Example: --no-acpi is only valid for QEMU_ARCH_I386.
>
> Which means 'query-command-line-options' should not report 'no-acpi'
> except when built for i386 emulation.

Yes.

>> 2. Options specific to the host OS are recognized by
>>    os_parse_cmd_args().  Any of them not recognized by the host OS's
>>    os_parse_cmd_args() are silently ignored.  *boggle*
>> 
>>    Example: --runas is ignored by the Windows build.
>
> Sounds like a bug.  Libvirt doesn't yet run qemu on a Windows build, but
> ideally, 'query-command-line-options' should not report 'runas' on a
> qemu binary built for windows.

Yes.

>> 3. Options depending on configuration are handled in (at least) three
>>    ways:
>> 
>>    a. The option is only recognized #ifdef CONFIG_FOO.  Which means it's
>>       silently ignored when FOO isn't enabled.  *boggle again*
>> 
>>       Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.
>
> Eww.  That's a bug - advertising a feature only to silently ignore it is
> not helpful.

Indeed.

>>    b. The option is always recognized, but rejected when #ifndef
>>       CONFIG_FOO.
>> 
>>       Example: --curses is rejected #ifndef CONFIG_CURSES.
>
> Recognizing and rejecting with a nice message, vs. not recognizing and
> giving a generic 'unknown option' message, both have the same net
> effect.  It's more code to give the nice message, and is helpful to
> humans, but if the option is omitted from 'query-command-line-options',
> it makes no difference to libvirt.

Yes.

>>    c. The option is always recognized, but rejected when its
>>       QemuOptsList hasn't been registered.  Essentially just an #if-less
>>       way to do 3b.
>> 
>>       Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is
>>       compiled in.
>> 
>> In my opinion, silently ignoring an option is a bug until proven
>> otherwise.
>
> Agreed.
>
>> 
>> Whether a non-sensical option should be recognized and rejected, or just
>> not recognized is debatable.
>
> Less code to not recognize it, you are then up to the question of
> whether the nicer error message warrants the extra code.
>
>> 
>> Regardless, telling QMP clients that an option works when it's always
>> rejected isn't useful.  We can either hide them in QMP, or add suitable
>> "invalid" markers, possibly identifying the reason.  Let's hide, unless
>> somebody can come up with a convincing use case for the additional
>> information.
>
> Agreed - hiding is nicer than having to expose yet more QMP details to
> mark an option that is "recognized but will never work".

Let's keep it simple until there's a clear need for fancy.

>> For each of the cases above, how can we hide?  
>> 
>> 1. Easy, check the target mask.
>> 
>> 2. Turning "makes sense for host OS" from code into data we can check.
>> 
>> 3a. Likewise.
>> 
>> 3b. Likewise.
>> 
>> 3c. If qemu-options.hx declares the QemuOptsList name, we can check
>>     whether the named list exists.  Could also be used to factor the
>>     qemu_opt_parse() out of the option switch.
>> 
>> We may not be able to get this wrapped in time for 2.0.  I'm not opposed
>> to a partial solution in 2.0, but let's think through the full solution,
>> to ensure the partial solution doesn't conflict with it.
>
> We're no worse than we were for 1.5 when query-command-line-options was
> first introduced - at this point, since we're not fixing a regression
> and since the bug is longstanding, it may be wiser to just leave 2.0
> as-is and save all the work for 2.1, rather than rushing in a partial
> fix for 2.0 only to have to redo it again later.

Let's aim for 2.1 then.

Amos, thanks for trying so hard to get it fixed in 2.0.  Us finding one
can of worms after the other is not your fault.

[Qemu-devel] [PATCH v5 for 2.0 2/3] update names in option tables to match with actual command-line spelling

[Amos Kong]

We want to establish a mapping between option name and option table,
then we can search related option table by option name.

This patch makes all the member name of QemuOptsList to match with
actual command-line spelling(option name).

[ Important Note ]

The QemuOptsList member name values are ABI, changing them can break
existing -readconfig configuration files.

This patch changes:

    from        to          introduced in
    acpi        acpitable   0c764a9 v1.5.0
    boot-opts   boot        3d3b830 v1.0
    smp-opts    smp         12b7f57 v1.6.0

All three have calcified into ABI already.

I have updated the release note of 2.0
http://wiki.qemu.org/ChangeLog/2.0#ABI_breaking

Signed-off-by: Amos Kong <akong@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/acpi/core.c        |  8 ++++----
 hw/nvram/fw_cfg.c     |  4 ++--
 include/qemu/option.h |  2 +-
 vl.c                  | 14 +++++++-------
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 79414b4..12e9ae8 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -54,16 +54,16 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
 char unsigned *acpi_tables;
 size_t acpi_tables_len;
 
-static QemuOptsList qemu_acpi_opts = {
-    .name = "acpi",
+static QemuOptsList qemu_acpitable_opts = {
+    .name = "acpitable",
     .implied_opt_name = "data",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpi_opts.head),
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpitable_opts.head),
     .desc = { { 0 } } /* validated with OptsVisitor */
 };
 
 static void acpi_register_config(void)
 {
-    qemu_add_opts(&qemu_acpi_opts);
+    qemu_add_opts(&qemu_acpitable_opts);
 }
 
 machine_init(acpi_register_config);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 282341a..3e6b048 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -125,7 +125,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
     const char *temp;
 
     /* get user configuration */
-    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOptsList *plist = qemu_find_opts("boot");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
     if (opts != NULL) {
         temp = qemu_opt_get(opts, "splash");
@@ -191,7 +191,7 @@ static void fw_cfg_reboot(FWCfgState *s)
     const char *temp;
 
     /* get user configuration */
-    QemuOptsList *plist = qemu_find_opts("boot-opts");
+    QemuOptsList *plist = qemu_find_opts("boot");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
     if (opts != NULL) {
         temp = qemu_opt_get(opts, "reboot-timeout");
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 8c0ac34..96b7c29 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -102,7 +102,7 @@ typedef struct QemuOptDesc {
 } QemuOptDesc;
 
 struct QemuOptsList {
-    const char *name;
+    const char *name;  /* option name */
     const char *implied_opt_name;
     bool merge_lists;  /* Merge multiple uses of option into a single list? */
     QTAILQ_HEAD(, QemuOpts) head;
diff --git a/vl.c b/vl.c
index 596ecfa..0464494 100644
--- a/vl.c
+++ b/vl.c
@@ -388,7 +388,7 @@ static QemuOptsList qemu_machine_opts = {
 };
 
 static QemuOptsList qemu_boot_opts = {
-    .name = "boot-opts",
+    .name = "boot",
     .implied_opt_name = "order",
     .merge_lists = true,
     .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
@@ -1359,7 +1359,7 @@ static void numa_add(const char *optarg)
 }
 
 static QemuOptsList qemu_smp_opts = {
-    .name = "smp-opts",
+    .name = "smp",
     .implied_opt_name = "cpus",
     .merge_lists = true,
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smp_opts.head),
@@ -3241,7 +3241,7 @@ int main(int argc, char **argv, char **envp)
                 drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
                 break;
             case QEMU_OPTION_boot:
-                opts = qemu_opts_parse(qemu_find_opts("boot-opts"), optarg, 1);
+                opts = qemu_opts_parse(qemu_find_opts("boot"), optarg, 1);
                 if (!opts) {
                     exit(1);
                 }
@@ -3610,7 +3610,7 @@ int main(int argc, char **argv, char **envp)
                 break;
             }
             case QEMU_OPTION_acpitable:
-                opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1);
+                opts = qemu_opts_parse(qemu_find_opts("acpitable"), optarg, 1);
                 if (!opts) {
                     exit(1);
                 }
@@ -3677,7 +3677,7 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_smp:
-                if (!qemu_opts_parse(qemu_find_opts("smp-opts"), optarg, 1)) {
+                if (!qemu_opts_parse(qemu_find_opts("smp"), optarg, 1)) {
                     exit(1);
                 }
                 break;
@@ -4002,7 +4002,7 @@ int main(int argc, char **argv, char **envp)
         data_dir[data_dir_idx++] = CONFIG_QEMU_DATADIR;
     }
 
-    smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
+    smp_parse(qemu_opts_find(qemu_find_opts("smp"), NULL));
 
     machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
     if (smp_cpus > machine->max_cpus) {
@@ -4186,7 +4186,7 @@ int main(int argc, char **argv, char **envp)
     bios_name = qemu_opt_get(machine_opts, "firmware");
 
     boot_order = machine->default_boot_order;
-    opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
+    opts = qemu_opts_find(qemu_find_opts("boot"), NULL);
     if (opts) {
         char *normal_boot_order;
         const char *order, *once;
-- 
1.8.5.3

[Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Amos Kong]

All the options are defined in qemu-options.hx. If we can't find a
matched option definition by group name of option table, then the
group name doesn't match with defined option name, it's not allowed
from 2.0

Signed-off-by: Amos Kong <akong@redhat.com>
---
 qemu-options.h     | 12 ++++++++++++
 util/qemu-config.c | 28 ++++++++++++++++++++++++++++
 vl.c               | 19 ++-----------------
 3 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/qemu-options.h b/qemu-options.h
index 89a009e..4024487 100644
--- a/qemu-options.h
+++ b/qemu-options.h
@@ -28,9 +28,21 @@
 #ifndef _QEMU_OPTIONS_H_
 #define _QEMU_OPTIONS_H_
 
+#include "sysemu/arch_init.h"
+
 enum {
 #define QEMU_OPTIONS_GENERATE_ENUM
 #include "qemu-options-wrapper.h"
 };
 
+#define HAS_ARG 0x0001
+
+typedef struct QEMUOption {
+    const char *name;
+    int flags;
+    int index;
+    uint32_t arch_mask;
+} QEMUOption;
+
+extern const QEMUOption qemu_options[];
 #endif
diff --git a/util/qemu-config.c b/util/qemu-config.c
index f610101..eba5428 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -6,6 +6,14 @@
 #include "hw/qdev.h"
 #include "qapi/error.h"
 #include "qmp-commands.h"
+#include "qemu-options.h"
+
+const QEMUOption qemu_options[] = {
+    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
+#define QEMU_OPTIONS_GENERATE_OPTIONS
+#include "qemu-options-wrapper.h"
+    { NULL },
+};
 
 static QemuOptsList *vm_config_groups[32];
 static QemuOptsList *drive_config_groups[4];
@@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list)
     abort();
 }
 
+/* check if the option is defined in qemu-options.hx */
+static bool opt_is_defined(const char *name)
+{
+    int i;
+
+    for (i = 0; qemu_options[i].name; i++) {
+        if (!strcmp(qemu_options[i].name, name)) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 void qemu_add_opts(QemuOptsList *list)
 {
     int entries, i;
@@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
     for (i = 0; i < entries; i++) {
         if (vm_config_groups[i] == NULL) {
             vm_config_groups[i] = list;
+            if (!opt_is_defined(list->name)) {
+                error_report("Didn't find a matched option definition, "
+                             "group name (%s) of option table must match with "
+                             "defined option name (Since 2.0)", list->name);
+                abort();
+            }
             return;
         }
     }
diff --git a/vl.c b/vl.c
index 0464494..bd44c52 100644
--- a/vl.c
+++ b/vl.c
@@ -111,7 +111,6 @@ int main(int argc, char **argv)
 #include "trace/control.h"
 #include "qemu/queue.h"
 #include "sysemu/cpus.h"
-#include "sysemu/arch_init.h"
 #include "qemu/osdep.h"
 
 #include "ui/qemu-spice.h"
@@ -2082,22 +2081,6 @@ static void help(int exitcode)
     exit(exitcode);
 }
 
-#define HAS_ARG 0x0001
-
-typedef struct QEMUOption {
-    const char *name;
-    int flags;
-    int index;
-    uint32_t arch_mask;
-} QEMUOption;
-
-static const QEMUOption qemu_options[] = {
-    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
-#define QEMU_OPTIONS_GENERATE_OPTIONS
-#include "qemu-options-wrapper.h"
-    { NULL },
-};
-
 static bool vga_available(void)
 {
     return object_class_by_name("VGA") || object_class_by_name("isa-vga");
@@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
     return popt;
 }
 
+#undef HAS_ARG
+
 static gpointer malloc_and_trace(gsize n_bytes)
 {
     void *ptr = malloc(n_bytes);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Leandro Dorileo]

Hi Amos,

On Thu, Mar 27, 2014 at 09:04:31PM +0800, Amos Kong wrote:
> All the options are defined in qemu-options.hx. If we can't find a
> matched option definition by group name of option table, then the
> group name doesn't match with defined option name, it's not allowed
> from 2.0
> 
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-options.h     | 12 ++++++++++++
>  util/qemu-config.c | 28 ++++++++++++++++++++++++++++
>  vl.c               | 19 ++-----------------
>  3 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-options.h b/qemu-options.h
> index 89a009e..4024487 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -28,9 +28,21 @@
>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> +#include "sysemu/arch_init.h"
> +
>  enum {
>  #define QEMU_OPTIONS_GENERATE_ENUM
>  #include "qemu-options-wrapper.h"
>  };
>  
> +#define HAS_ARG 0x0001
> +
> +typedef struct QEMUOption {
> +    const char *name;
> +    int flags;
> +    int index;
> +    uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index f610101..eba5428 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,14 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +const QEMUOption qemu_options[] = {
> +    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +    { NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list)
>      abort();
>  }
>  
> +/* check if the option is defined in qemu-options.hx */
> +static bool opt_is_defined(const char *name)
> +{
> +    int i;
> +
> +    for (i = 0; qemu_options[i].name; i++) {
> +        if (!strcmp(qemu_options[i].name, name)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  void qemu_add_opts(QemuOptsList *list)
>  {
>      int entries, i;
> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>      for (i = 0; i < entries; i++) {
>          if (vm_config_groups[i] == NULL) {
>              vm_config_groups[i] = list;
> +            if (!opt_is_defined(list->name)) {
> +                error_report("Didn't find a matched option definition, "
> +                             "group name (%s) of option table must match with "
> +                             "defined option name (Since 2.0)", list->name);


I'm not a native English speaker but I don't fell comfortable with this
message output, what about "option table's group name (%s) must match with
predefined option name (Since 2.0)"?

The patch looks good, I tested it and it works properly, so

Reviewed-by: Leandro Dorileo <l@dorileo.org>



> +                abort();
> +            }
>              return;
>          }
>      }
> diff --git a/vl.c b/vl.c
> index 0464494..bd44c52 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,6 @@ int main(int argc, char **argv)
>  #include "trace/control.h"
>  #include "qemu/queue.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/arch_init.h"
>  #include "qemu/osdep.h"
>  
>  #include "ui/qemu-spice.h"
> @@ -2082,22 +2081,6 @@ static void help(int exitcode)
>      exit(exitcode);
>  }
>  
> -#define HAS_ARG 0x0001
> -
> -typedef struct QEMUOption {
> -    const char *name;
> -    int flags;
> -    int index;
> -    uint32_t arch_mask;
> -} QEMUOption;
> -
> -static const QEMUOption qemu_options[] = {
> -    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> -#define QEMU_OPTIONS_GENERATE_OPTIONS
> -#include "qemu-options-wrapper.h"
> -    { NULL },
> -};
> -
>  static bool vga_available(void)
>  {
>      return object_class_by_name("VGA") || object_class_by_name("isa-vga");
> @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>      return popt;
>  }
>  
> +#undef HAS_ARG
> +
>  static gpointer malloc_and_trace(gsize n_bytes)
>  {
>      void *ptr = malloc(n_bytes);
> -- 
> 1.8.5.3
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Markus Armbruster]

Amos Kong <akong@redhat.com> writes:

> All the options are defined in qemu-options.hx. If we can't find a
> matched option definition by group name of option table, then the
> group name doesn't match with defined option name, it's not allowed
> from 2.0
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  qemu-options.h     | 12 ++++++++++++
>  util/qemu-config.c | 28 ++++++++++++++++++++++++++++
>  vl.c               | 19 ++-----------------
>  3 files changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/qemu-options.h b/qemu-options.h
> index 89a009e..4024487 100644
> --- a/qemu-options.h
> +++ b/qemu-options.h
> @@ -28,9 +28,21 @@
>  #ifndef _QEMU_OPTIONS_H_
>  #define _QEMU_OPTIONS_H_
>  
> +#include "sysemu/arch_init.h"
> +
>  enum {
>  #define QEMU_OPTIONS_GENERATE_ENUM
>  #include "qemu-options-wrapper.h"
>  };
>  
> +#define HAS_ARG 0x0001
> +
> +typedef struct QEMUOption {
> +    const char *name;
> +    int flags;
> +    int index;
> +    uint32_t arch_mask;
> +} QEMUOption;
> +
> +extern const QEMUOption qemu_options[];
>  #endif

Instead of exposing struct QEMUOption and qemu_options[] here, why not
simply put opt_is_defined() in vl.c, and declare it in qemu-options.h?

> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index f610101..eba5428 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -6,6 +6,14 @@
>  #include "hw/qdev.h"
>  #include "qapi/error.h"
>  #include "qmp-commands.h"
> +#include "qemu-options.h"
> +
> +const QEMUOption qemu_options[] = {
> +    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> +#define QEMU_OPTIONS_GENERATE_OPTIONS
> +#include "qemu-options-wrapper.h"
> +    { NULL },
> +};
>  
>  static QemuOptsList *vm_config_groups[32];
>  static QemuOptsList *drive_config_groups[4];
> @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list)
>      abort();
>  }
>  
> +/* check if the option is defined in qemu-options.hx */
> +static bool opt_is_defined(const char *name)
> +{
> +    int i;
> +
> +    for (i = 0; qemu_options[i].name; i++) {
> +        if (!strcmp(qemu_options[i].name, name)) {
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +

The same loop is hidden in lookup_opt().  We could avoid the duplication
by putting it in a function returning the option index.  That could
easily be a proper enumeration type, like this, in qemu-options.h:

typedef enum QEMUOption {
#define QEMU_OPTIONS_GENERATE_ENUM
#include "qemu-options-wrapper.h"
} QEMUOption;

QEMUOption qemu_option_lookup(const char *name);

plus a rename of vl.c's QEMUOption to QEMUOptionDesc or something.

Can be done on top.

>  void qemu_add_opts(QemuOptsList *list)
>  {
>      int entries, i;
> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>      for (i = 0; i < entries; i++) {
>          if (vm_config_groups[i] == NULL) {
>              vm_config_groups[i] = list;
> +            if (!opt_is_defined(list->name)) {
> +                error_report("Didn't find a matched option definition, "
> +                             "group name (%s) of option table must match with "
> +                             "defined option name (Since 2.0)", list->name);
> +                abort();
> +            }
>              return;
>          }
>      }

Simple!  Wish it was my idea ;)

Why not simply assert(opt_is_defined(list->name))?

> diff --git a/vl.c b/vl.c
> index 0464494..bd44c52 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -111,7 +111,6 @@ int main(int argc, char **argv)
>  #include "trace/control.h"
>  #include "qemu/queue.h"
>  #include "sysemu/cpus.h"
> -#include "sysemu/arch_init.h"
>  #include "qemu/osdep.h"
>  
>  #include "ui/qemu-spice.h"
> @@ -2082,22 +2081,6 @@ static void help(int exitcode)
>      exit(exitcode);
>  }
>  
> -#define HAS_ARG 0x0001
> -
> -typedef struct QEMUOption {
> -    const char *name;
> -    int flags;
> -    int index;
> -    uint32_t arch_mask;
> -} QEMUOption;
> -
> -static const QEMUOption qemu_options[] = {
> -    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> -#define QEMU_OPTIONS_GENERATE_OPTIONS
> -#include "qemu-options-wrapper.h"
> -    { NULL },
> -};
> -
>  static bool vga_available(void)
>  {
>      return object_class_by_name("VGA") || object_class_by_name("isa-vga");
> @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
>      return popt;
>  }
>  
> +#undef HAS_ARG
> +
>  static gpointer malloc_and_trace(gsize n_bytes)
>  {
>      void *ptr = malloc(n_bytes);

Undefining HAS_ARG here, where it hasn't done any harm, while letting it
pollute every other compilation unit including qemu-options.h makes no
sense.

Re: [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

On 03/28/2014 08:55 AM, Markus Armbruster wrote:
> Amos Kong <akong@redhat.com> writes:
> 
>> All the options are defined in qemu-options.hx. If we can't find a
>> matched option definition by group name of option table, then the
>> group name doesn't match with defined option name, it's not allowed
>> from 2.0
>>

>> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>>      for (i = 0; i < entries; i++) {
>>          if (vm_config_groups[i] == NULL) {
>>              vm_config_groups[i] = list;
>> +            if (!opt_is_defined(list->name)) {
>> +                error_report("Didn't find a matched option definition, "
>> +                             "group name (%s) of option table must match with "
>> +                             "defined option name (Since 2.0)", list->name);
>> +                abort();
>> +            }
>>              return;
>>          }
>>      }
> 
> Simple!  Wish it was my idea ;)
> 
> Why not simply assert(opt_is_defined(list->name))?

Indeed, using assert() would also solve the problem of the error message
being awkward.

>>  
>> -#define HAS_ARG 0x0001
>> -

>> -
>> -static const QEMUOption qemu_options[] = {
>> -    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
>> -#define QEMU_OPTIONS_GENERATE_OPTIONS
>> -#include "qemu-options-wrapper.h"
>> -    { NULL },
>> -};

>>  
>> +#undef HAS_ARG

HAS_ARG is not very namespace clean.  Prior to your patch, it was used
only in a single file (where we know it doesn't collide).  After your
patch, it is now in a header used by multiple files.

>> +
>>  static gpointer malloc_and_trace(gsize n_bytes)
>>  {
>>      void *ptr = malloc(n_bytes);
> 
> Undefining HAS_ARG here, where it hasn't done any harm, while letting it
> pollute every other compilation unit including qemu-options.h makes no
> sense.

Maybe a better approach would be to create an enum in qemu-options.h of
actual flag values:

typedef enum {
    QEMU_OPT_HAS_ARG = 1,
} QEMUOptionFlags;

and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c.  Additionally, you
either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or
you can take a shortcut in qemu-config.c:

#define HAS_ARG QEMU_OPT_HAS_ARG

const QEMUOption qemu_options[] = {
    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
#define QEMU_OPTIONS_GENERATE_OPTIONS
#include "qemu-options-wrapper.h"
    { NULL },
};

#undef HAS_ARG

since that is the only place that includes the .hx file at a point where
HAS_ARG has to be expanded to something useful.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 for 2.0 3/3] abort QEMU if group name in option table doesn't match with defined option name

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/28/2014 08:55 AM, Markus Armbruster wrote:
>> Amos Kong <akong@redhat.com> writes:
>> 
>>> All the options are defined in qemu-options.hx. If we can't find a
>>> matched option definition by group name of option table, then the
>>> group name doesn't match with defined option name, it's not allowed
>>> from 2.0
>>>
>
>>> @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list)
>>>      for (i = 0; i < entries; i++) {
>>>          if (vm_config_groups[i] == NULL) {
>>>              vm_config_groups[i] = list;
>>> +            if (!opt_is_defined(list->name)) {
>>> +                error_report("Didn't find a matched option definition, "
>>> +                             "group name (%s) of option table must match with "
>>> +                             "defined option name (Since 2.0)", list->name);
>>> +                abort();
>>> +            }
>>>              return;
>>>          }
>>>      }
>> 
>> Simple!  Wish it was my idea ;)
>> 
>> Why not simply assert(opt_is_defined(list->name))?
>
> Indeed, using assert() would also solve the problem of the error message
> being awkward.
>
>>>  
>>> -#define HAS_ARG 0x0001
>>> -
>
>>> -
>>> -static const QEMUOption qemu_options[] = {
>>> -    { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
>>> -#define QEMU_OPTIONS_GENERATE_OPTIONS
>>> -#include "qemu-options-wrapper.h"
>>> -    { NULL },
>>> -};
>
>>>  
>>> +#undef HAS_ARG
>
> HAS_ARG is not very namespace clean.  Prior to your patch, it was used
> only in a single file (where we know it doesn't collide).  After your
> patch, it is now in a header used by multiple files.
>
>>> +
>>>  static gpointer malloc_and_trace(gsize n_bytes)
>>>  {
>>>      void *ptr = malloc(n_bytes);
>> 
>> Undefining HAS_ARG here, where it hasn't done any harm, while letting it
>> pollute every other compilation unit including qemu-options.h makes no
>> sense.
>
> Maybe a better approach would be to create an enum in qemu-options.h of
> actual flag values:
>
> typedef enum {
>     QEMU_OPT_HAS_ARG = 1,
> } QEMUOptionFlags;
>
> and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c.  Additionally, you
> either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or
> you can take a shortcut in qemu-config.c:
>
> #define HAS_ARG QEMU_OPT_HAS_ARG
>
> const QEMUOption qemu_options[] = {
>     { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL },
> #define QEMU_OPTIONS_GENERATE_OPTIONS
> #include "qemu-options-wrapper.h"
>     { NULL },
> };
>
> #undef HAS_ARG
>
> since that is the only place that includes the .hx file at a point where
> HAS_ARG has to be expanded to something useful.

Either something like this, or avoid moving this stuff to a header.  I
prefer the latter, and suggested how in my review.