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

28 Mar 2014


      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

    