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 <l@dorileo.org>]

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