Re: [Qemu-devel] [PATCH V26 01/32] QemuOpts: move find_desc_by_name ahead for later calling

Re: [Qemu-devel] [PATCH V26 01/32] QemuOpts: move find_desc_by_name ahead for later calling

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:10PM +0800, Chunyan Liu wrote:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>


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


> ---
>  util/qemu-option.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 8bbc3ad..808aef4 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -173,6 +173,20 @@ static void parse_option_number(const char *name, const char *value,
>      }
>  }
>  
> +static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
> +                                            const char *name)
> +{
> +    int i;
> +
> +    for (i = 0; desc[i].name != NULL; i++) {
> +        if (strcmp(desc[i].name, name) == 0) {
> +            return &desc[i];
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
>  void parse_option_size(const char *name, const char *value,
>                         uint64_t *ret, Error **errp)
>  {
> @@ -637,20 +651,6 @@ 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)
> -{
> -    int i;
> -
> -    for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> -            return &desc[i];
> -        }
> -    }
> -
> -    return NULL;
> -}
> -
>  int qemu_opt_unset(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 02/32] QemuOpts: add def_value_str to QemuOptDesc

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:11PM +0800, Chunyan Liu wrote:
> Add def_value_str (default value) to QemuOptDesc, to replace function of the
> default value in QEMUOptionParameter.
> 
> Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise,
> if desc->def_value_str is set, return desc->def_value_str; otherwise, return
> input defval.
> 
> Improve qemu_opts_print: if option is set, print opt->str; otherwise, if
> desc->def_value_str is set, also print it. It will replace
> print_option_parameters.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>


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


> ---
> Changes to V25:
>   * split v25 patch into two: this one and next one.
>   * this patch is the same as v22 which is reviewed-by Eric.
> 
>  include/qemu/option.h |  3 ++-
>  util/qemu-option.c    | 56 ++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 8c0ac34..c3b0a91 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -99,6 +99,7 @@ typedef struct QemuOptDesc {
>      const char *name;
>      enum QemuOptType type;
>      const char *help;
> +    const char *def_value_str;
>  } QemuOptDesc;
>  
>  struct QemuOptsList {
> @@ -156,7 +157,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict);
>  void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp);
>  
>  typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
> -int qemu_opts_print(QemuOpts *opts, void *dummy);
> +void qemu_opts_print(QemuOpts *opts);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure);
>  
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 808aef4..5346c90 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -570,6 +570,13 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +
> +    if (!opt) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            return desc->def_value_str;
> +        }
> +    }
>      return opt ? opt->str : NULL;
>  }
>  
> @@ -589,8 +596,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_bool(name, desc->def_value_str, &defval, &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
>      return opt->value.boolean;
>  }
> @@ -599,8 +611,14 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_number(name, desc->def_value_str, &defval,
> +                                &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
>      return opt->value.uint;
>  }
> @@ -609,8 +627,13 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
>  
> -    if (opt == NULL)
> +    if (opt == NULL) {
> +        const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            parse_option_size(name, desc->def_value_str, &defval, &error_abort);
> +        }
>          return defval;
> +    }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
>      return opt->value.uint;
>  }
> @@ -895,17 +918,32 @@ void qemu_opts_del(QemuOpts *opts)
>      g_free(opts);
>  }
>  
> -int qemu_opts_print(QemuOpts *opts, void *dummy)
> +void qemu_opts_print(QemuOpts *opts)
>  {
>      QemuOpt *opt;
> +    QemuOptDesc *desc = opts->list->desc;
>  
> -    fprintf(stderr, "%s: %s:", opts->list->name,
> -            opts->id ? opts->id : "<noid>");
> -    QTAILQ_FOREACH(opt, &opts->head, next) {
> -        fprintf(stderr, " %s=\"%s\"", opt->name, opt->str);
> +    if (desc[0].name == NULL) {
> +        QTAILQ_FOREACH(opt, &opts->head, next) {
> +            fprintf(stderr, "%s=\"%s\" ", opt->name, opt->str);
> +        }
> +        return;
> +    }
> +    for (; desc && desc->name; desc++) {
> +        const char *value;
> +        QemuOpt *opt = qemu_opt_find(opts, desc->name);
> +
> +        value = opt ? opt->str : desc->def_value_str;
> +        if (!value) {
> +            continue;
> +        }
> +        if (desc->type == QEMU_OPT_STRING) {
> +            fprintf(stderr, "%s='%s' ", desc->name, value);
> +        } else {
> +            fprintf(stderr, "%s=%s ", desc->name, value);
> +        }
>      }
>      fprintf(stderr, "\n");
> -    return 0;
>  }
>  
>  static int opts_do_parse(QemuOpts *opts, const char *params,
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 03/32] QemuOpts: repurpose qemu_opts_print to replace print_option_parameters

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:12PM +0800, Chunyan Liu wrote:
> Currently this function is not used anywhere. In later patches, it will
> replace print_option_parameters. To avoid print info changes, change
> qemu_opts_print from fprintf stderr to printf to keep consistent with
> print_option_parameters, remove last printf and print size/number with
> opt->value.uint instead of opt->str.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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

> ---
>  util/qemu-option.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 5346c90..2be6995 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -925,7 +925,7 @@ void qemu_opts_print(QemuOpts *opts)
>  
>      if (desc[0].name == NULL) {
>          QTAILQ_FOREACH(opt, &opts->head, next) {
> -            fprintf(stderr, "%s=\"%s\" ", opt->name, opt->str);
> +            printf("%s=\"%s\" ", opt->name, opt->str);
>          }
>          return;
>      }
> @@ -938,12 +938,14 @@ void qemu_opts_print(QemuOpts *opts)
>              continue;
>          }
>          if (desc->type == QEMU_OPT_STRING) {
> -            fprintf(stderr, "%s='%s' ", desc->name, value);
> +            printf("%s='%s' ", desc->name, value);
> +        } else if ((desc->type == QEMU_OPT_SIZE ||
> +                    desc->type == QEMU_OPT_NUMBER) && opt) {
> +            printf("%s=%" PRId64 " ", desc->name, opt->value.uint);
>          } else {
> -            fprintf(stderr, "%s=%s ", desc->name, value);
> +            printf("%s=%s ", desc->name, value);
>          }
>      }
> -    fprintf(stderr, "\n");
>  }
>  
>  static int opts_do_parse(QemuOpts *opts, const char *params,
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 04/32] qapi: output def_value_str when query command line options

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:13PM +0800, Chunyan Liu wrote:
> Change qapi interfaces to output the newly added def_value_str when querying
> command line options.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>


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


> ---
> Changes to V25:
>   * update @default description in .json file
> 
>  qapi-schema.json   | 5 ++++-
>  qmp-commands.hx    | 2 ++
>  util/qemu-config.c | 4 ++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0b00427..880829d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4088,12 +4088,15 @@
>  #
>  # @help: #optional human readable text string, not suitable for parsing.
>  #
> +# @default: #optional default value string (since 2.1)
> +#
>  # Since 1.5
>  ##
>  { 'type': 'CommandLineParameterInfo',
>    'data': { 'name': 'str',
>              'type': 'CommandLineParameterType',
> -            '*help': 'str' } }
> +            '*help': 'str',
> +            '*default': 'str' } }
>  
>  ##
>  # @CommandLineOptionInfo:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ed3ab92..1271332 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2895,6 +2895,8 @@ Each array entry contains the following:
>                or 'size')
>      - "help": human readable description of the parameter
>                (json-string, optional)
> +    - "default": default value string for the parameter
> +                 (json-string, optional)
>  
>  Example:
>  
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index f4e4f38..ba375c0 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -82,6 +82,10 @@ static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
>              info->has_help = true;
>              info->help = g_strdup(desc[i].help);
>          }
> +        if (desc[i].def_value_str) {
> +            info->has_q_default = true;
> +            info->q_default = g_strdup(desc[i].def_value_str);
> +        }
>  
>          entry = g_malloc0(sizeof(*entry));
>          entry->value = info;
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 05/32] QemuOpts: change opt->name|str from (const char *) to (char *)

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:14PM +0800, Chunyan Liu wrote:
> qemu_opt_del() already assumes that all QemuOpt instances contain
> malloc'd name and value; but it had to cast away const because
> opts_start_struct() was doing its own thing and using static storage
> instead.  By using the correct type and malloced strings everywhere, the
> usage of this struct becomes clearer.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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


> ---
>  include/qemu/option_int.h |  4 ++--
>  qapi/opts-visitor.c       | 10 +++++++---
>  util/qemu-option.c        |  4 ++--
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
> index 8212fa4..6432c1a 100644
> --- a/include/qemu/option_int.h
> +++ b/include/qemu/option_int.h
> @@ -30,8 +30,8 @@
>  #include "qemu/error-report.h"
>  
>  struct QemuOpt {
> -    const char   *name;
> -    const char   *str;
> +    char *name;
> +    char *str;
>  
>      const QemuOptDesc *desc;
>      union {
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 5d830a2..7a64f4e 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -143,8 +143,8 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
>      if (ov->opts_root->id != NULL) {
>          ov->fake_id_opt = g_malloc0(sizeof *ov->fake_id_opt);
>  
> -        ov->fake_id_opt->name = "id";
> -        ov->fake_id_opt->str = ov->opts_root->id;
> +        ov->fake_id_opt->name = g_strdup("id");
> +        ov->fake_id_opt->str = g_strdup(ov->opts_root->id);
>          opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
>      }
>  }
> @@ -177,7 +177,11 @@ opts_end_struct(Visitor *v, Error **errp)
>      }
>      g_hash_table_destroy(ov->unprocessed_opts);
>      ov->unprocessed_opts = NULL;
> -    g_free(ov->fake_id_opt);
> +    if (ov->fake_id_opt) {
> +        g_free(ov->fake_id_opt->name);
> +        g_free(ov->fake_id_opt->str);
> +        g_free(ov->fake_id_opt);
> +    }
>      ov->fake_id_opt = NULL;
>  }
>  
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 2be6995..69cdf3f 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -664,8 +664,8 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>  static void qemu_opt_del(QemuOpt *opt)
>  {
>      QTAILQ_REMOVE(&opt->opts->head, opt, next);
> -    g_free((/* !const */ char*)opt->name);
> -    g_free((/* !const */ char*)opt->str);
> +    g_free(opt->name);
> +    g_free(opt->str);
>      g_free(opt);
>  }
>  
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 06/32] QemuOpts: move qemu_opt_del ahead for later calling

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:15PM +0800, Chunyan Liu wrote:
> In later patch, qemu_opt_get_del functions will be added, they will
> first get the option value, then call qemu_opt_del to remove the option
> from opt list. To prepare for that purpose, move qemu_opt_del ahead first.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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

> ---
>  util/qemu-option.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 69cdf3f..4d2d4d1 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -567,6 +567,14 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>      return NULL;
>  }
>  
> +static void qemu_opt_del(QemuOpt *opt)
> +{
> +    QTAILQ_REMOVE(&opt->opts->head, opt, next);
> +    g_free(opt->name);
> +    g_free(opt->str);
> +    g_free(opt);
> +}
> +
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> @@ -661,14 +669,6 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>      }
>  }
>  
> -static void qemu_opt_del(QemuOpt *opt)
> -{
> -    QTAILQ_REMOVE(&opt->opts->head, opt, next);
> -    g_free(opt->name);
> -    g_free(opt->str);
> -    g_free(opt);
> -}
> -
>  static bool opts_accepts_any(const QemuOpts *opts)
>  {
>      return opts->list->desc[0].name == NULL;
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 07/32] QemuOpts: add qemu_opt_get_*_del functions for replace work

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:16PM +0800, Chunyan Liu wrote:
> Add qemu_opt_get_del, qemu_opt_get_bool_del, qemu_opt_get_number_del and
> qemu_opt_get_size_del to replace the same handling of QEMUOptionParameter
> (get and delete).
> 
> Several drivers are coded to parse a known subset of options, then
> remove them from the list before handing all remaining options to a
> second driver for further option processing.  get_*_del makes it easier
> to retrieve a known option (or its default) and remove it from the list
> all in one action.
> 
> Share common helper function:
> 
> For qemu_opt_get_bool/size/number, they and their get_*_del counterpart
> could share most of the code except whether or not deleting the opt from
> option list, so generate common helper functions.
> 
> For qemu_opt_get and qemu_opt_get_del, keep code duplication, since
> 1. qemu_opt_get_del returns malloc'd memory while qemu_opt_get returns
> in-place memory
> 2. qemu_opt_get_del returns (char *), qemu_opt_get returns (const char *),
> and could not change to (char *), since in one case, it will return
> desc->def_value_str, which is (const char *).
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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

> ---
>  include/qemu/option.h |   6 +++
>  util/qemu-option.c    | 116 ++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 109 insertions(+), 13 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index c3b0a91..6653e43 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -111,6 +111,7 @@ struct QemuOptsList {
>  };
>  
>  const char *qemu_opt_get(QemuOpts *opts, const char *name);
> +char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>  /**
>   * qemu_opt_has_help_opt:
>   * @opts: options to search for a help request
> @@ -126,6 +127,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts);
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
>  uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>  uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
> +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> +                                 uint64_t defval);
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> +                               uint64_t defval);
>  int qemu_opt_unset(QemuOpts *opts, const char *name);
>  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
>  void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 4d2d4d1..32e1d50 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -575,6 +575,19 @@ static void qemu_opt_del(QemuOpt *opt)
>      g_free(opt);
>  }
>  
> +/* qemu_opt_set allows many settings for the same option.
> + * This function deletes all settings for an option.
> + */
> +static void qemu_opt_del_all(QemuOpts *opts, const char *name)
> +{
> +    QemuOpt *opt, *next_opt;
> +
> +    QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) {
> +        if (!strcmp(opt->name, name))
> +            qemu_opt_del(opt);
> +    }
> +}
> +
>  const char *qemu_opt_get(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> @@ -588,6 +601,34 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>      return opt ? opt->str : NULL;
>  }
>  
> +/* Get a known option (or its default) and remove it from the list
> + * all in one action. Return a malloced string of the option value.
> + * Result must be freed by caller with g_free().
> + */
> +char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> +{
> +    QemuOpt *opt;
> +    const QemuOptDesc *desc;
> +    char *str = NULL;
> +
> +    if (opts == NULL) {
> +        return NULL;
> +    }
> +
> +    opt = qemu_opt_find(opts, name);
> +    if (!opt) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            str = g_strdup(desc->def_value_str);
> +        }
> +        return str;
> +    }
> +    str = opt->str;
> +    opt->str = NULL;
> +    qemu_opt_del_all(opts, name);
> +    return str;
> +}
> +
>  bool qemu_opt_has_help_opt(QemuOpts *opts)
>  {
>      QemuOpt *opt;
> @@ -600,50 +641,99 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>      return false;
>  }
>  
> -bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> +static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name,
> +                                     bool defval, bool del)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    bool ret = defval;
>  
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_bool(name, desc->def_value_str, &defval, &error_abort);
> +            parse_option_bool(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;
>      }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
> -    return opt->value.boolean;
> +    ret = opt->value.boolean;
> +    if (del) {
> +        qemu_opt_del_all(opts, name);
> +    }
> +    return ret;
>  }
>  
> -uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> +bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
> +{
> +    return qemu_opt_get_bool_helper(opts, name, defval, false);
> +}
> +
> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
> +{
> +    return qemu_opt_get_bool_helper(opts, name, defval, true);
> +}
> +
> +static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name,
> +                                           uint64_t defval, bool del)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    uint64_t ret = defval;
>  
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_number(name, desc->def_value_str, &defval,
> -                                &error_abort);
> +            parse_option_number(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;
>      }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER);
> -    return opt->value.uint;
> +    ret = opt->value.uint;
> +    if (del) {
> +        qemu_opt_del_all(opts, name);
> +    }
> +    return ret;
>  }
>  
> -uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> +uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
> +{
> +    return qemu_opt_get_number_helper(opts, name, defval, false);
> +}
> +
> +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name,
> +                                 uint64_t defval)
> +{
> +    return qemu_opt_get_number_helper(opts, name, defval, true);
> +}
> +
> +static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name,
> +                                         uint64_t defval, bool del)
>  {
>      QemuOpt *opt = qemu_opt_find(opts, name);
> +    uint64_t ret = defval;
>  
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> -            parse_option_size(name, desc->def_value_str, &defval, &error_abort);
> +            parse_option_size(name, desc->def_value_str, &ret, &error_abort);
>          }
> -        return defval;
> +        return ret;
>      }
>      assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
> -    return opt->value.uint;
> +    ret = opt->value.uint;
> +    if (del) {
> +        qemu_opt_del_all(opts, name);
> +    }
> +    return ret;
> +}
> +
> +uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
> +{
> +    return qemu_opt_get_size_helper(opts, name, defval, false);
> +}
> +
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> +                               uint64_t defval)
> +{
> +    return qemu_opt_get_size_helper(opts, name, defval, true);
>  }
>  
>  static void qemu_opt_parse(QemuOpt *opt, Error **errp)
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 08/32] QemuOpts: add qemu_opts_print_help to replace print_option_help

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:17PM +0800, Chunyan Liu wrote:
> print_option_help takes QEMUOptionParameter as parameter, add
> qemu_opts_print_help to take QemuOptsList as parameter for later
> replace work.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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

> ---
>  include/qemu/option.h |  1 +
>  util/qemu-option.c    | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 6653e43..fbf5dc2 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -166,5 +166,6 @@ typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque);
>  void qemu_opts_print(QemuOpts *opts);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure);
> +void qemu_opts_print_help(QemuOptsList *list);
>  
>  #endif
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 32e1d50..adb7c3c 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -553,6 +553,19 @@ void print_option_help(QEMUOptionParameter *list)
>      }
>  }
>  
> +void qemu_opts_print_help(QemuOptsList *list)
> +{
> +    QemuOptDesc *desc;
> +
> +    assert(list);
> +    desc = list->desc;
> +    printf("Supported options:\n");
> +    while (desc && desc->name) {
> +        printf("%-16s %s\n", desc->name,
> +               desc->help ? desc->help : "No description available");
> +        desc++;
> +    }
> +}
>  /* ------------------------------------------------------------------ */
>  
>  static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 09/32] QemuOpts: add conversion between QEMUOptionParameter to QemuOpts

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:18PM +0800, Chunyan Liu wrote:
> Add two temp conversion functions between QEMUOptionParameter to QemuOpts,
> so that next patch can use it. It will simplify later patch for easier
> review. And will be finally removed after all backend drivers switch to
> QemuOpts.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>


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


> ---
>  include/qemu/option.h |   9 +++
>  util/qemu-option.c    | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 162 insertions(+)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index fbf5dc2..e98e8ef 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -103,6 +103,12 @@ typedef struct QemuOptDesc {
>  } QemuOptDesc;
>  
>  struct QemuOptsList {
> +    /* FIXME: Temp used for QEMUOptionParamter->QemuOpts conversion to
> +     * indicate the need to free memory. Will remove after all drivers
> +     * switch to QemuOpts.
> +     */
> +    bool allocated;
> +
>      const char *name;
>      const char *implied_opt_name;
>      bool merge_lists;  /* Merge multiple uses of option into a single list? */
> @@ -167,5 +173,8 @@ void qemu_opts_print(QemuOpts *opts);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>                        int abort_on_failure);
>  void qemu_opts_print_help(QemuOptsList *list);
> +void qemu_opts_free(QemuOptsList *list);
> +QEMUOptionParameter *opts_to_params(QemuOpts *opts);
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list);
>  
>  #endif
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index adb7c3c..93ffcd5 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -1345,3 +1345,156 @@ int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
>      loc_pop(&loc);
>      return rc;
>  }
> +
> +static size_t count_opts_list(QemuOptsList *list)
> +{
> +    QemuOptDesc *desc = NULL;
> +    size_t num_opts = 0;
> +
> +    if (!list) {
> +        return 0;
> +    }
> +
> +    desc = list->desc;
> +    while (desc && desc->name) {
> +        num_opts++;
> +        desc++;
> +    }
> +
> +    return num_opts;
> +}
> +
> +/* Convert QEMUOptionParameter to QemuOpts
> + * FIXME: this function will be removed after all drivers
> + * switch to QemuOpts
> + */
> +QemuOptsList *params_to_opts(QEMUOptionParameter *list)
> +{
> +    QemuOptsList *opts = NULL;
> +    size_t num_opts, i = 0;
> +
> +    if (!list) {
> +        return NULL;
> +    }
> +
> +    num_opts = count_option_parameters(list);
> +    opts = g_malloc0(sizeof(QemuOptsList) +
> +                     (num_opts + 1) * sizeof(QemuOptDesc));
> +    QTAILQ_INIT(&opts->head);
> +    /* (const char *) members will point to malloced space and need to free */
> +    opts->allocated = true;
> +
> +    while (list && list->name) {
> +        opts->desc[i].name = g_strdup(list->name);
> +        opts->desc[i].help = g_strdup(list->help);
> +        switch (list->type) {
> +        case OPT_FLAG:
> +            opts->desc[i].type = QEMU_OPT_BOOL;
> +            opts->desc[i].def_value_str =
> +                g_strdup(list->value.n ? "on" : "off");
> +            break;
> +
> +        case OPT_NUMBER:
> +            opts->desc[i].type = QEMU_OPT_NUMBER;
> +            if (list->value.n) {
> +                opts->desc[i].def_value_str =
> +                    g_strdup_printf("%" PRIu64, list->value.n);
> +            }
> +            break;
> +
> +        case OPT_SIZE:
> +            opts->desc[i].type = QEMU_OPT_SIZE;
> +            if (list->value.n) {
> +                opts->desc[i].def_value_str =
> +                    g_strdup_printf("%" PRIu64, list->value.n);
> +            }
> +            break;
> +
> +        case OPT_STRING:
> +            opts->desc[i].type = QEMU_OPT_STRING;
> +            opts->desc[i].def_value_str = g_strdup(list->value.s);
> +            break;
> +        }
> +
> +        i++;
> +        list++;
> +    }
> +
> +    return opts;
> +}
> +
> +/* convert QemuOpts to QEMUOptionParameter
> + * Note: result QEMUOptionParameter has shorter lifetime than
> + * input QemuOpts.
> + * FIXME: this function will be removed after all drivers
> + * switch to QemuOpts
> + */
> +QEMUOptionParameter *opts_to_params(QemuOpts *opts)
> +{
> +    QEMUOptionParameter *dest = NULL;
> +    QemuOptDesc *desc;
> +    size_t num_opts, i = 0;
> +    const char *tmp;
> +
> +    if (!opts || !opts->list || !opts->list->desc) {
> +        return NULL;
> +    }
> +    assert(!opts_accepts_any(opts));
> +
> +    num_opts = count_opts_list(opts->list);
> +    dest = g_malloc0((num_opts + 1) * sizeof(QEMUOptionParameter));
> +
> +    desc = opts->list->desc;
> +    while (desc && desc->name) {
> +        dest[i].name = desc->name;
> +        dest[i].help = desc->help;
> +        dest[i].assigned = qemu_opt_find(opts, desc->name) ? true : false;
> +        switch (desc->type) {
> +        case QEMU_OPT_STRING:
> +            dest[i].type = OPT_STRING;
> +            tmp = qemu_opt_get(opts, desc->name);
> +            dest[i].value.s = g_strdup(tmp);
> +            break;
> +
> +        case QEMU_OPT_BOOL:
> +            dest[i].type = OPT_FLAG;
> +            dest[i].value.n = qemu_opt_get_bool(opts, desc->name, 0) ? 1 : 0;
> +            break;
> +
> +        case QEMU_OPT_NUMBER:
> +            dest[i].type = OPT_NUMBER;
> +            dest[i].value.n = qemu_opt_get_number(opts, desc->name, 0);
> +            break;
> +
> +        case QEMU_OPT_SIZE:
> +            dest[i].type = OPT_SIZE;
> +            dest[i].value.n = qemu_opt_get_size(opts, desc->name, 0);
> +            break;
> +        }
> +
> +        i++;
> +        desc++;
> +    }
> +
> +    return dest;
> +}
> +
> +void qemu_opts_free(QemuOptsList *list)
> +{
> +    /* List members point to new malloced space and need to be freed.
> +     * FIXME:
> +     * Introduced for QEMUOptionParamter->QemuOpts conversion.
> +     * Will remove after all drivers switch to QemuOpts.
> +     */
> +    if (list && list->allocated) {
> +        QemuOptDesc *desc = list->desc;
> +        while (desc && desc->name) {
> +            g_free((char *)desc->name);
> +            g_free((char *)desc->help);
> +            g_free((char *)desc->def_value_str);
> +            desc++;
> +        }
> +    }
> +
> +    g_free(list);
> +}
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 11/32] QemuOpts: check NULL input for qemu_opts_del

[Leandro Dorileo]

On Tue, Apr 29, 2014 at 05:08:20PM +0800, Chunyan Liu wrote:
> To simplify later using of qemu_opts_del, accept NULL input.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>

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

> ---
>  util/qemu-option.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 4de99b3..2667e16 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -1010,6 +1010,10 @@ void qemu_opts_del(QemuOpts *opts)
>  {
>      QemuOpt *opt;
>  
> +    if (opts == NULL) {
> +        return;
> +    }
> +
>      for (;;) {
>          opt = QTAILQ_FIRST(&opts->head);
>          if (opt == NULL)
> -- 
> 1.8.4.5
> 
> 

-- 
Leandro Dorileo

Re: [Qemu-devel] [PATCH V26 12/32] change block layer to support both QemuOpts and QEMUOptionParamter

[Leandro Dorileo]

Chunyan,

On Tue, Apr 29, 2014 at 05:08:21PM +0800, Chunyan Liu wrote:
> Change block layer to support both QemuOpts and QEMUOptionParameter.
> After this patch, it will change backend drivers one by one. At the end,
> QEMUOptionParameter will be removed and only QemuOpts is kept.


This patch breaks the tests in the intermediate tree state, when you clean things
up in the end of your series most problems this patch introduces are removed as well,
I saw problems with 061 which I could easily find the problem (see below) and then
I saw an error on 063 with a mem corruption (this last one I had no time to investigate
more).

Your patch series is looking very good, just take some time to make sure you're not
breaking the tests and the build in the intermediate states (on all you patches).


> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> Changes to V25:
>   * fix Eric's comments:
>   * update bdrv_create_co_entry and bdrv_amend_options code, to let it
>     more readable.
>   * add assertion in bdrv_register.
>   * improve comments to create_opts in header file.
> 
>  block.c                   | 158 ++++++++++++++++++++++++++++++++--------------
>  block/cow.c               |   2 +-
>  block/qcow.c              |   2 +-
>  block/qcow2.c             |   2 +-
>  block/qed.c               |   2 +-
>  block/raw_bsd.c           |   2 +-
>  block/vhdx.c              |   2 +-
>  block/vmdk.c              |   4 +-
>  block/vvfat.c             |   2 +-
>  include/block/block.h     |   7 +-
>  include/block/block_int.h |  13 +++-
>  qemu-img.c                |  94 +++++++++++++--------------
>  12 files changed, 180 insertions(+), 110 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4745712..124f045 100644
> --- a/block.c
> +++ b/block.c
> @@ -328,6 +328,13 @@ void bdrv_register(BlockDriver *bdrv)
>          }
>      }
>  
> +    if (bdrv->bdrv_create) {
> +        assert(!bdrv->bdrv_create2 && !bdrv->create_opts);
> +        assert(!bdrv->bdrv_amend_options2);
> +    } else if (bdrv->bdrv_create2) {
> +        assert(!bdrv->bdrv_create && !bdrv->create_options);
> +        assert(!bdrv->bdrv_amend_options);
> +    }
>      QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list);
>  }
>  
> @@ -419,6 +426,7 @@ typedef struct CreateCo {
>      BlockDriver *drv;
>      char *filename;
>      QEMUOptionParameter *options;
> +    QemuOpts *opts;
>      int ret;
>      Error *err;
>  } CreateCo;
> @@ -430,8 +438,28 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  
>      CreateCo *cco = opaque;
>      assert(cco->drv);
> +    assert(!(cco->options && cco->opts));
>  
> -    ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +    if (cco->drv->bdrv_create2) {
> +        QemuOptsList *opts_list = NULL;
> +        if (cco->options) {
> +            opts_list = params_to_opts(cco->options);
> +            cco->opts = qemu_opts_create(opts_list, NULL, 0, &error_abort);
> +        }
> +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err);
> +        if (cco->options) {
> +            qemu_opts_del(cco->opts);
> +            qemu_opts_free(opts_list);
> +        }
> +    } else {
> +        if (cco->opts) {
> +            cco->options = opts_to_params(cco->opts);
> +        }
> +        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
> +        if (cco->opts) {
> +            free_option_parameters(cco->options);
> +        }
> +    }
>      if (local_err) {
>          error_propagate(&cco->err, local_err);
>      }
> @@ -439,7 +467,8 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
>  }
>  
>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp)
> +                QEMUOptionParameter *options,
> +                QemuOpts *opts, Error **errp)
>  {
>      int ret;
>  
> @@ -448,11 +477,12 @@ int bdrv_create(BlockDriver *drv, const char* filename,
>          .drv = drv,
>          .filename = g_strdup(filename),
>          .options = options,
> +        .opts = opts,
>          .ret = NOT_DONE,
>          .err = NULL,
>      };
>  
> -    if (!drv->bdrv_create) {
> +    if (!drv->bdrv_create && !drv->bdrv_create2) {
>          error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
>          ret = -ENOTSUP;
>          goto out;
> @@ -484,7 +514,7 @@ out:
>  }
>  
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> -                     Error **errp)
> +                     QemuOpts *opts, Error **errp)
>  {
>      BlockDriver *drv;
>      Error *local_err = NULL;
> @@ -496,7 +526,7 @@ int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
>          return -ENOENT;
>      }
>  
> -    ret = bdrv_create(drv, filename, options, &local_err);
> +    ret = bdrv_create(drv, filename, options, opts, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> @@ -1184,7 +1214,8 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
>      int64_t total_size;
>      BlockDriver *bdrv_qcow2;
> -    QEMUOptionParameter *create_options;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      QDict *snapshot_options;
>      BlockDriverState *bs_snapshot;
>      Error *local_err;
> @@ -1209,13 +1240,20 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
>      }
>  
>      bdrv_qcow2 = bdrv_find_format("qcow2");
> -    create_options = parse_option_parameters("", bdrv_qcow2->create_options,
> -                                             NULL);
> -
> -    set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size);
>  
> -    ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err);
> -    free_option_parameters(create_options);
> +    assert(!(bdrv_qcow2->create_options && bdrv_qcow2->create_opts));
> +    if (bdrv_qcow2->create_options) {
> +        create_opts = params_to_opts(bdrv_qcow2->create_options);
> +    } else {
> +        create_opts = bdrv_qcow2->create_opts;
> +    }
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size);
> +    ret = bdrv_create(bdrv_qcow2, tmp_filename, NULL, opts, &local_err);
> +    qemu_opts_del(opts);
> +    if (bdrv_qcow2->create_options) {
> +        qemu_opts_free(create_opts);
> +    }
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not create temporary overlay "
>                           "'%s': %s", tmp_filename,
> @@ -5310,8 +5348,10 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                       char *options, uint64_t img_size, int flags,
>                       Error **errp, bool quiet)
>  {
> -    QEMUOptionParameter *param = NULL, *create_options = NULL;
> -    QEMUOptionParameter *backing_fmt, *backing_file, *size;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
> +    const char *backing_fmt, *backing_file;
> +    int64_t size;
>      BlockDriver *drv, *proto_drv;
>      BlockDriver *backing_drv = NULL;
>      Error *local_err = NULL;
> @@ -5330,28 +5370,25 @@ void bdrv_img_create(const char *filename, const char *fmt,
>          return;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                   proto_drv->create_options);
>  
>      /* Create parameter list with default values */
> -    param = parse_option_parameters("", create_options, param);
> -
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size);
>  
>      /* Parse -o options */
>      if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> +        if (qemu_opts_do_parse(opts, options, NULL) != 0) {
>              error_setg(errp, "Invalid options for file format '%s'.", fmt);
>              goto out;
>          }
>      }
>  
>      if (base_filename) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
> -                                 base_filename)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
>              error_setg(errp, "Backing file not supported for file format '%s'",
>                         fmt);
>              goto out;
> @@ -5359,37 +5396,37 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>      if (base_fmt) {
> -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_setg(errp, "Backing file format not supported for file "
>                               "format '%s'", fmt);
>              goto out;
>          }
>      }
>  
> -    backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> -    if (backing_file && backing_file->value.s) {
> -        if (!strcmp(filename, backing_file->value.s)) {
> +    backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
> +    if (backing_file) {
> +        if (!strcmp(filename, backing_file)) {
>              error_setg(errp, "Error: Trying to create an image with the "
>                               "same filename as the backing file");
>              goto out;
>          }
>      }
>  
> -    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT);
> -    if (backing_fmt && backing_fmt->value.s) {
> -        backing_drv = bdrv_find_format(backing_fmt->value.s);
> +    backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT);
> +    if (backing_fmt) {
> +        backing_drv = bdrv_find_format(backing_fmt);
>          if (!backing_drv) {
>              error_setg(errp, "Unknown backing file format '%s'",
> -                       backing_fmt->value.s);
> +                       backing_fmt);
>              goto out;
>          }
>      }
>  
>      // The size for the image must always be specified, with one exception:
>      // If we are using a backing file, we can obtain the size from there
> -    size = get_option_parameter(param, BLOCK_OPT_SIZE);
> -    if (size && size->value.n == -1) {
> -        if (backing_file && backing_file->value.s) {
> +    size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> +    if (size == -1) {
> +        if (backing_file) {
>              BlockDriverState *bs;
>              uint64_t size;
>              char buf[32];
> @@ -5400,11 +5437,11 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>  
>              bs = NULL;
> -            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL, back_flags,
> +            ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags,
>                              backing_drv, &local_err);
>              if (ret < 0) {
>                  error_setg_errno(errp, -ret, "Could not open '%s': %s",
> -                                 backing_file->value.s,
> +                                 backing_file,
>                                   error_get_pretty(local_err));
>                  error_free(local_err);
>                  local_err = NULL;
> @@ -5414,7 +5451,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>              size *= 512;
>  
>              snprintf(buf, sizeof(buf), "%" PRId64, size);
> -            set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size);
>  
>              bdrv_unref(bs);
>          } else {
> @@ -5425,16 +5462,18 @@ void bdrv_img_create(const char *filename, const char *fmt,
>  
>      if (!quiet) {
>          printf("Formatting '%s', fmt=%s ", filename, fmt);
> -        print_option_parameters(param);
> +        qemu_opts_print(opts);
>          puts("");
>      }
> -    ret = bdrv_create(drv, filename, param, &local_err);
> +
> +    ret = bdrv_create(drv, filename, NULL, opts, &local_err);
> +
>      if (ret == -EFBIG) {
>          /* This is generally a better message than whatever the driver would
>           * deliver (especially because of the cluster_size_hint), since that
>           * is most probably not much different from "image too large". */
>          const char *cluster_size_hint = "";
> -        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
> +        if (qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE, 0)) {
>              cluster_size_hint = " (try using a larger cluster size)";
>          }
>          error_setg(errp, "The image size is too large for file format '%s'"
> @@ -5444,9 +5483,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      }
>  
>  out:
> -    free_option_parameters(create_options);
> -    free_option_parameters(param);
> -
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> @@ -5464,12 +5502,36 @@ void bdrv_add_before_write_notifier(BlockDriverState *bs,
>      notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
>  }
>  
> -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
> +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options,
> +                       QemuOpts *opts)
>  {
> -    if (bs->drv->bdrv_amend_options == NULL) {
> +    int ret;
> +    assert(!(options && opts));
> +
> +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) {
>          return -ENOTSUP;
>      }
> -    return bs->drv->bdrv_amend_options(bs, options);
> +    if (bs->drv->bdrv_amend_options2) {
> +        QemuOptsList *opts_list = NULL;
> +        if (options) {
> +            opts_list = params_to_opts(options);
> +            opts = qemu_opts_create(opts_list, NULL, 0, &error_abort);
> +        }
> +        ret = bs->drv->bdrv_amend_options2(bs, opts);
> +        if (options) {
> +            qemu_opts_del(opts);
> +            qemu_opts_free(opts_list);
> +        }
> +    } else {
> +        if (opts) {
> +            options = opts_to_params(opts);
> +        }
> +        ret = bs->drv->bdrv_amend_options(bs, options);
> +        if (opts) {
> +            free_option_parameters(options);
> +        }
> +    }
> +    return ret;
>  }
>  
>  /* This function will be called by the bdrv_recurse_is_first_non_filter method
> diff --git a/block/cow.c b/block/cow.c
> index 30deb88..26cf4a5 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -345,7 +345,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
>          options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qcow.c b/block/qcow.c
> index d5a7d5f..9411aef 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -687,7 +687,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
>          options++;
>      }
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e903d97..49985e3 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1625,7 +1625,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/qed.c b/block/qed.c
> index c130e42..2982640 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -566,7 +566,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
>      int ret = 0;
>      BlockDriverState *bs;
>  
> -    ret = bdrv_create_file(filename, NULL, &local_err);
> +    ret = bdrv_create_file(filename, NULL, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          return ret;
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 01ea692..9ae5fc2 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -145,7 +145,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      Error *local_err = NULL;
>      int ret;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>      }
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 509baaf..a9fcf6b 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1796,7 +1796,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
>      block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX :
>                                                      block_size;
>  
> -    ret = bdrv_create_file(filename, options, &local_err);
> +    ret = bdrv_create_file(filename, options, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 06a1f9f..3802863 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1526,7 +1526,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>      uint32_t *gd_buf = NULL;
>      int gd_buf_size;
>  
> -    ret = bdrv_create_file(filename, NULL, &local_err);
> +    ret = bdrv_create_file(filename, NULL, NULL, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto exit;
> @@ -1866,7 +1866,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
>      if (!split && !flat) {
>          desc_offset = 0x200;
>      } else {
> -        ret = bdrv_create_file(filename, options, &local_err);
> +        ret = bdrv_create_file(filename, options, NULL, &local_err);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not create image file");
>              goto exit;
> diff --git a/block/vvfat.c b/block/vvfat.c
> index c3af7ff..155fc9b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2926,7 +2926,7 @@ static int enable_write_target(BDRVVVFATState *s)
>      set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
>      set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
>  
> -    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err);
> +    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, NULL, &local_err);
>      if (ret < 0) {
>          qerror_report_err(local_err);
>          error_free(local_err);
> diff --git a/include/block/block.h b/include/block/block.h
> index c12808a..bfa2192 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -177,9 +177,9 @@ BlockDriver *bdrv_find_format(const char *format_name);
>  BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
>                                            bool readonly);
>  int bdrv_create(BlockDriver *drv, const char* filename,
> -    QEMUOptionParameter *options, Error **errp);
> +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp);
>  int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
> -                     Error **errp);
> +                     QemuOpts *opts, Error **errp);
>  BlockDriverState *bdrv_new(const char *device_name, Error **errp);
>  void bdrv_make_anon(BlockDriverState *bs);
>  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
> @@ -284,7 +284,8 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> -int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
> +int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options,
> +                       QemuOpts *opts);
>  
>  /* external snapshots */
>  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index cd5bc73..59875db 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -118,6 +118,8 @@ struct BlockDriver {
>      void (*bdrv_rebind)(BlockDriverState *bs);
>      int (*bdrv_create)(const char *filename, QEMUOptionParameter *options,
>                         Error **errp);
> +    /* FIXME: will remove the duplicate and rename back to bdrv_create later */
> +    int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error **errp);
>      int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
>      int (*bdrv_make_empty)(BlockDriverState *bs);
>      /* aio */
> @@ -217,7 +219,12 @@ struct BlockDriver {
>  
>      /* List of options for creating images, terminated by name == NULL */
>      QEMUOptionParameter *create_options;
> -
> +    /* FIXME: will replace create_options.
> +     * These two fields are mutually exclusive. At most one is non-NULL.
> +     * create_options should only be set with bdrv_create, and create_opts
> +     * should only be set with bdrv_create2.
> +     */
> +    QemuOptsList *create_opts;
>  
>      /*
>       * Returns 0 for completed check, -errno for internal errors.
> @@ -228,6 +235,10 @@ struct BlockDriver {
>  
>      int (*bdrv_amend_options)(BlockDriverState *bs,
>          QEMUOptionParameter *options);
> +    /* FIXME: will remove the duplicate and rename back to
> +     * bdrv_amend_options later
> +     */
> +    int (*bdrv_amend_options2)(BlockDriverState *bs, QemuOpts *opts);
>  
>      void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 968b4c8..14d3acd 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -249,7 +249,7 @@ static int read_password(char *buf, int buf_size)
>  static int print_block_option_help(const char *filename, const char *fmt)
>  {
>      BlockDriver *drv, *proto_drv;
> -    QEMUOptionParameter *create_options = NULL;
> +    QemuOptsList *create_opts = NULL;
>  
>      /* Find driver and parse its options */
>      drv = bdrv_find_format(fmt);
> @@ -258,21 +258,20 @@ static int print_block_option_help(const char *filename, const char *fmt)
>          return 1;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
>      if (filename) {
>          proto_drv = bdrv_find_protocol(filename, true);
>          if (!proto_drv) {
>              error_report("Unknown protocol '%s'", filename);
>              return 1;
>          }
> -        create_options = append_option_parameters(create_options,
> -                                                  proto_drv->create_options);
> +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                       proto_drv->create_options);
>      }
>  
> -    print_option_help(create_options);
> -    free_option_parameters(create_options);
> +    qemu_opts_print_help(create_opts);
> +    qemu_opts_free(create_opts);
>      return 0;
>  }
>  
> @@ -326,19 +325,19 @@ fail:
>      return NULL;
>  }
>  
> -static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
> +static int add_old_style_options(const char *fmt, QemuOpts *opts,
>                                   const char *base_filename,
>                                   const char *base_fmt)
>  {
>      if (base_filename) {
> -        if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) {
>              error_report("Backing file not supported for file format '%s'",
>                           fmt);
>              return -1;
>          }
>      }
>      if (base_fmt) {
> -        if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) {
> +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) {
>              error_report("Backing file format not supported for file "
>                           "format '%s'", fmt);
>              return -1;
> @@ -1169,8 +1168,9 @@ static int img_convert(int argc, char **argv)
>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
>      BlockDriverInfo bdi;
> -    QEMUOptionParameter *param = NULL, *create_options = NULL;
> -    QEMUOptionParameter *out_baseimg_param;
> +    QemuOpts *opts = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    const char *out_baseimg_param;
>      char *options = NULL;
>      const char *snapshot_name = NULL;
>      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
> @@ -1359,40 +1359,36 @@ static int img_convert(int argc, char **argv)
>          goto out;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -                                              drv->create_options);
> -    create_options = append_option_parameters(create_options,
> -                                              proto_drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, drv->create_opts,
> +                                   drv->create_options);
> +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts,
> +                                   proto_drv->create_options);
>  
> -    if (options) {
> -        param = parse_option_parameters(options, create_options, param);
> -        if (param == NULL) {
> -            error_report("Invalid options for file format '%s'.", out_fmt);
> -            ret = -1;
> -            goto out;
> -        }
> -    } else {
> -        param = parse_option_parameters("", create_options, param);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    if (options && qemu_opts_do_parse(opts, options, NULL)) {
> +        error_report("Invalid options for file format '%s'.", out_fmt);
> +        ret = -1;
> +        goto out;
>      }
>  
> -    set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512);
> -    ret = add_old_style_options(out_fmt, param, out_baseimg, NULL);
> +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512);
> +    ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
>      if (ret < 0) {
>          goto out;
>      }
>  
>      /* Get backing file name if -o backing_file was used */
> -    out_baseimg_param = get_option_parameter(param, BLOCK_OPT_BACKING_FILE);
> +    out_baseimg_param = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE);
>      if (out_baseimg_param) {
> -        out_baseimg = out_baseimg_param->value.s;
> +        out_baseimg = out_baseimg_param;
>      }
>  
>      /* Check if compression is supported */
>      if (compress) {
> -        QEMUOptionParameter *encryption =
> -            get_option_parameter(param, BLOCK_OPT_ENCRYPT);
> -        QEMUOptionParameter *preallocation =
> -            get_option_parameter(param, BLOCK_OPT_PREALLOC);
> +        bool encryption =
> +            qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false);
> +        const char *preallocation =
> +            qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
>  
>          if (!drv->bdrv_write_compressed) {
>              error_report("Compression not supported for this file format");
> @@ -1400,15 +1396,15 @@ static int img_convert(int argc, char **argv)
>              goto out;
>          }
>  
> -        if (encryption && encryption->value.n) {
> +        if (encryption) {
>              error_report("Compression and encryption not supported at "
>                           "the same time");
>              ret = -1;
>              goto out;
>          }
>  
> -        if (preallocation && preallocation->value.s
> -            && strcmp(preallocation->value.s, "off"))
> +        if (preallocation
> +            && strcmp(preallocation, "off"))
>          {
>              error_report("Compression and preallocation not supported at "
>                           "the same time");
> @@ -1419,7 +1415,7 @@ static int img_convert(int argc, char **argv)
>  
>      if (!skip_create) {
>          /* Create the new image */
> -        ret = bdrv_create(drv, out_filename, param, &local_err);
> +        ret = bdrv_create(drv, out_filename, NULL, opts, &local_err);
>          if (ret < 0) {
>              error_report("%s: error while converting %s: %s",
>                           out_filename, out_fmt, error_get_pretty(local_err));
> @@ -1683,8 +1679,8 @@ out:
>          qemu_progress_print(100, 0);
>      }
>      qemu_progress_end();
> -    free_option_parameters(create_options);
> -    free_option_parameters(param);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      qemu_vfree(buf);
>      if (sn_opts) {
>          qemu_opts_del(sn_opts);
> @@ -2675,7 +2671,8 @@ static int img_amend(int argc, char **argv)
>  {
>      int c, ret = 0;
>      char *options = NULL;
> -    QEMUOptionParameter *create_options = NULL, *options_param = NULL;
> +    QemuOptsList *create_opts = NULL;
> +    QemuOpts *opts = NULL;
>      const char *fmt = NULL, *filename;
>      bool quiet = false;
>      BlockDriverState *bs = NULL;
> @@ -2746,17 +2743,16 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>  
> -    create_options = append_option_parameters(create_options,
> -            bs->drv->create_options);
> -    options_param = parse_option_parameters(options, create_options,
> -            options_param);
> -    if (options_param == NULL) {
> +    create_opts = qemu_opts_append(create_opts, bs->drv->create_opts,
> +                                   bs->drv->create_options);
> +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
> +    if (options && qemu_opts_do_parse(opts, options, NULL)) {


Since now we're calling qemu_opts_do_parse() instead of parse_option_parameters()
you should update the 061 test output, otherwise the test will fail due to wrong
output, the following should be enough:

--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -281,7 +281,7 @@ Lazy refcounts only supported with compatibility level 1.1 and above (use compat
 qemu-img: Error while amending options: Invalid argument
 Unknown compatibility level 0.42.
 qemu-img: Error while amending options: Invalid argument
-Unknown option 'foo'
+qemu-img: Invalid parameter 'foo'
 qemu-img: Invalid options for file format 'qcow2'
 Changing the cluster size is not supported.
 qemu-img: Error while amending options: Operation not supported

Regards...

-- 
Leandro Dorileo


>          error_report("Invalid options for file format '%s'", fmt);
>          ret = -1;
>          goto out;
>      }
>  
> -    ret = bdrv_amend_options(bs, options_param);
> +    ret = bdrv_amend_options(bs, NULL, opts);
>      if (ret < 0) {
>          error_report("Error while amending options: %s", strerror(-ret));
>          goto out;
> @@ -2766,8 +2762,8 @@ out:
>      if (bs) {
>          bdrv_unref(bs);
>      }
> -    free_option_parameters(create_options);
> -    free_option_parameters(options_param);
> +    qemu_opts_del(opts);
> +    qemu_opts_free(create_opts);
>      g_free(options);
>  
>      if (ret) {
> -- 
> 1.8.4.5
> 
> 

Re: [Qemu-devel] [PATCH V26 12/32] change block layer to support both QemuOpts and QEMUOptionParamter

[Chun Yan Liu]

>>> On 5/2/2014 at 06:09 AM, in message
<20140501220948.GK29944@dorilex-lnv.MultilaserAP>, Leandro Dorileo
<l@dorileo.org> wrote: 
> Chunyan, 
>  
> On Tue, Apr 29, 2014 at 05:08:21PM +0800, Chunyan Liu wrote: 
> > Change block layer to support both QemuOpts and QEMUOptionParameter. 
> > After this patch, it will change backend drivers one by one. At the end, 
> > QEMUOptionParameter will be removed and only QemuOpts is kept. 
>  
>  
> This patch breaks the tests in the intermediate tree state, when you clean  
> things 
> up in the end of your series most problems this patch introduces are removed  
> as well, 
> I saw problems with 061 which I could easily find the problem (see below)  
> and then 
> I saw an error on 063 with a mem corruption (this last one I had no time to  
> investigate 
> more). 

Fix qemu_opts_append:
g_free(tmp_list) -> qemu_opts_free(tmp_list) 

> Your patch series is looking very good, just take some time to make sure  
> you're not 
> breaking the tests and the build in the intermediate states (on all you  
> patches). 
>  
>  
> >  
> > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> 
> > Signed-off-by: Chunyan Liu <cyliu@suse.com> 
> > --- 
> > Changes to V25: 
> >   * fix Eric's comments: 
> >   * update bdrv_create_co_entry and bdrv_amend_options code, to let it 
> >     more readable. 
> >   * add assertion in bdrv_register. 
> >   * improve comments to create_opts in header file. 
> >  
> >  block.c                   | 158 ++++++++++++++++++++++++++++++++-------------- 
> >  block/cow.c               |   2 +- 
> >  block/qcow.c              |   2 +- 
> >  block/qcow2.c             |   2 +- 
> >  block/qed.c               |   2 +- 
> >  block/raw_bsd.c           |   2 +- 
> >  block/vhdx.c              |   2 +- 
> >  block/vmdk.c              |   4 +- 
> >  block/vvfat.c             |   2 +- 
> >  include/block/block.h     |   7 +- 
> >  include/block/block_int.h |  13 +++- 
> >  qemu-img.c                |  94 +++++++++++++-------------- 
> >  12 files changed, 180 insertions(+), 110 deletions(-) 
> >  
> > diff --git a/block.c b/block.c 
> > index 4745712..124f045 100644 
> > --- a/block.c 
> > +++ b/block.c 
> > @@ -328,6 +328,13 @@ void bdrv_register(BlockDriver *bdrv) 
> >          } 
> >      } 
> >   
> > +    if (bdrv->bdrv_create) { 
> > +        assert(!bdrv->bdrv_create2 && !bdrv->create_opts); 
> > +        assert(!bdrv->bdrv_amend_options2); 
> > +    } else if (bdrv->bdrv_create2) { 
> > +        assert(!bdrv->bdrv_create && !bdrv->create_options); 
> > +        assert(!bdrv->bdrv_amend_options); 
> > +    } 
> >      QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list); 
> >  } 
> >   
> > @@ -419,6 +426,7 @@ typedef struct CreateCo { 
> >      BlockDriver *drv; 
> >      char *filename; 
> >      QEMUOptionParameter *options; 
> > +    QemuOpts *opts; 
> >      int ret; 
> >      Error *err; 
> >  } CreateCo; 
> > @@ -430,8 +438,28 @@ static void coroutine_fn bdrv_create_co_entry(void  
> *opaque) 
> >   
> >      CreateCo *cco = opaque; 
> >      assert(cco->drv); 
> > +    assert(!(cco->options && cco->opts)); 
> >   
> > -    ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); 
> > +    if (cco->drv->bdrv_create2) { 
> > +        QemuOptsList *opts_list = NULL; 
> > +        if (cco->options) { 
> > +            opts_list = params_to_opts(cco->options); 
> > +            cco->opts = qemu_opts_create(opts_list, NULL, 0, &error_abort); 
> > +        } 
> > +        ret = cco->drv->bdrv_create2(cco->filename, cco->opts, &local_err); 
> > +        if (cco->options) { 
> > +            qemu_opts_del(cco->opts); 
> > +            qemu_opts_free(opts_list); 
> > +        } 
> > +    } else { 
> > +        if (cco->opts) { 
> > +            cco->options = opts_to_params(cco->opts); 
> > +        } 
> > +        ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err); 
> > +        if (cco->opts) { 
> > +            free_option_parameters(cco->options); 
> > +        } 
> > +    } 
> >      if (local_err) { 
> >          error_propagate(&cco->err, local_err); 
> >      } 
> > @@ -439,7 +467,8 @@ static void coroutine_fn bdrv_create_co_entry(void  
> *opaque) 
> >  } 
> >   
> >  int bdrv_create(BlockDriver *drv, const char* filename, 
> > -    QEMUOptionParameter *options, Error **errp) 
> > +                QEMUOptionParameter *options, 
> > +                QemuOpts *opts, Error **errp) 
> >  { 
> >      int ret; 
> >   
> > @@ -448,11 +477,12 @@ int bdrv_create(BlockDriver *drv, const char*  
> filename, 
> >          .drv = drv, 
> >          .filename = g_strdup(filename), 
> >          .options = options, 
> > +        .opts = opts, 
> >          .ret = NOT_DONE, 
> >          .err = NULL, 
> >      }; 
> >   
> > -    if (!drv->bdrv_create) { 
> > +    if (!drv->bdrv_create && !drv->bdrv_create2) { 
> >          error_setg(errp, "Driver '%s' does not support image creation",  
> drv->format_name); 
> >          ret = -ENOTSUP; 
> >          goto out; 
> > @@ -484,7 +514,7 @@ out: 
> >  } 
> >   
> >  int bdrv_create_file(const char* filename, QEMUOptionParameter *options, 
> > -                     Error **errp) 
> > +                     QemuOpts *opts, Error **errp) 
> >  { 
> >      BlockDriver *drv; 
> >      Error *local_err = NULL; 
> > @@ -496,7 +526,7 @@ int bdrv_create_file(const char* filename,  
> QEMUOptionParameter *options, 
> >          return -ENOENT; 
> >      } 
> >   
> > -    ret = bdrv_create(drv, filename, options, &local_err); 
> > +    ret = bdrv_create(drv, filename, options, opts, &local_err); 
> >      if (local_err) { 
> >          error_propagate(errp, local_err); 
> >      } 
> > @@ -1184,7 +1214,8 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs,  
> Error **errp) 
> >      char *tmp_filename = g_malloc0(PATH_MAX + 1); 
> >      int64_t total_size; 
> >      BlockDriver *bdrv_qcow2; 
> > -    QEMUOptionParameter *create_options; 
> > +    QemuOptsList *create_opts = NULL; 
> > +    QemuOpts *opts = NULL; 
> >      QDict *snapshot_options; 
> >      BlockDriverState *bs_snapshot; 
> >      Error *local_err; 
> > @@ -1209,13 +1240,20 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs,  
> Error **errp) 
> >      } 
> >   
> >      bdrv_qcow2 = bdrv_find_format("qcow2"); 
> > -    create_options = parse_option_parameters("", bdrv_qcow2->create_options, 
> > -                                             NULL); 
> > - 
> > -    set_option_parameter_int(create_options, BLOCK_OPT_SIZE, total_size); 
> >   
> > -    ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err); 
> > -    free_option_parameters(create_options); 
> > +    assert(!(bdrv_qcow2->create_options && bdrv_qcow2->create_opts)); 
> > +    if (bdrv_qcow2->create_options) { 
> > +        create_opts = params_to_opts(bdrv_qcow2->create_options); 
> > +    } else { 
> > +        create_opts = bdrv_qcow2->create_opts; 
> > +    } 
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); 
> > +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_size); 
> > +    ret = bdrv_create(bdrv_qcow2, tmp_filename, NULL, opts, &local_err); 
> > +    qemu_opts_del(opts); 
> > +    if (bdrv_qcow2->create_options) { 
> > +        qemu_opts_free(create_opts); 
> > +    } 
> >      if (ret < 0) { 
> >          error_setg_errno(errp, -ret, "Could not create temporary overlay " 
> >                           "'%s': %s", tmp_filename, 
> > @@ -5310,8 +5348,10 @@ void bdrv_img_create(const char *filename, const char  
> *fmt, 
> >                       char *options, uint64_t img_size, int flags, 
> >                       Error **errp, bool quiet) 
> >  { 
> > -    QEMUOptionParameter *param = NULL, *create_options = NULL; 
> > -    QEMUOptionParameter *backing_fmt, *backing_file, *size; 
> > +    QemuOptsList *create_opts = NULL; 
> > +    QemuOpts *opts = NULL; 
> > +    const char *backing_fmt, *backing_file; 
> > +    int64_t size; 
> >      BlockDriver *drv, *proto_drv; 
> >      BlockDriver *backing_drv = NULL; 
> >      Error *local_err = NULL; 
> > @@ -5330,28 +5370,25 @@ void bdrv_img_create(const char *filename, const  
> char *fmt, 
> >          return; 
> >      } 
> >   
> > -    create_options = append_option_parameters(create_options, 
> > -                                              drv->create_options); 
> > -    create_options = append_option_parameters(create_options, 
> > -                                              proto_drv->create_options); 
> > +    create_opts = qemu_opts_append(create_opts, drv->create_opts, 
> > +                                   drv->create_options); 
> > +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, 
> > +                                   proto_drv->create_options); 
> >   
> >      /* Create parameter list with default values */ 
> > -    param = parse_option_parameters("", create_options, param); 
> > - 
> > -    set_option_parameter_int(param, BLOCK_OPT_SIZE, img_size); 
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); 
> > +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size); 
> >   
> >      /* Parse -o options */ 
> >      if (options) { 
> > -        param = parse_option_parameters(options, create_options, param); 
> > -        if (param == NULL) { 
> > +        if (qemu_opts_do_parse(opts, options, NULL) != 0) { 
> >              error_setg(errp, "Invalid options for file format '%s'.",  
> fmt); 
> >              goto out; 
> >          } 
> >      } 
> >   
> >      if (base_filename) { 
> > -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE, 
> > -                                 base_filename)) { 
> > +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) { 
> >              error_setg(errp, "Backing file not supported for file format  
> '%s'", 
> >                         fmt); 
> >              goto out; 
> > @@ -5359,37 +5396,37 @@ void bdrv_img_create(const char *filename, const  
> char *fmt, 
> >      } 
> >   
> >      if (base_fmt) { 
> > -        if (set_option_parameter(param, BLOCK_OPT_BACKING_FMT, base_fmt)) { 
> > +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) { 
> >              error_setg(errp, "Backing file format not supported for file " 
> >                               "format '%s'", fmt); 
> >              goto out; 
> >          } 
> >      } 
> >   
> > -    backing_file = get_option_parameter(param, BLOCK_OPT_BACKING_FILE); 
> > -    if (backing_file && backing_file->value.s) { 
> > -        if (!strcmp(filename, backing_file->value.s)) { 
> > +    backing_file = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); 
> > +    if (backing_file) { 
> > +        if (!strcmp(filename, backing_file)) { 
> >              error_setg(errp, "Error: Trying to create an image with the " 
> >                               "same filename as the backing file"); 
> >              goto out; 
> >          } 
> >      } 
> >   
> > -    backing_fmt = get_option_parameter(param, BLOCK_OPT_BACKING_FMT); 
> > -    if (backing_fmt && backing_fmt->value.s) { 
> > -        backing_drv = bdrv_find_format(backing_fmt->value.s); 
> > +    backing_fmt = qemu_opt_get(opts, BLOCK_OPT_BACKING_FMT); 
> > +    if (backing_fmt) { 
> > +        backing_drv = bdrv_find_format(backing_fmt); 
> >          if (!backing_drv) { 
> >              error_setg(errp, "Unknown backing file format '%s'", 
> > -                       backing_fmt->value.s); 
> > +                       backing_fmt); 
> >              goto out; 
> >          } 
> >      } 
> >   
> >      // The size for the image must always be specified, with one  
> exception: 
> >      // If we are using a backing file, we can obtain the size from there 
> > -    size = get_option_parameter(param, BLOCK_OPT_SIZE); 
> > -    if (size && size->value.n == -1) { 
> > -        if (backing_file && backing_file->value.s) { 
> > +    size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); 
> > +    if (size == -1) { 
> > +        if (backing_file) { 
> >              BlockDriverState *bs; 
> >              uint64_t size; 
> >              char buf[32]; 
> > @@ -5400,11 +5437,11 @@ void bdrv_img_create(const char *filename, const  
> char *fmt, 
> >                  flags & ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT |  
> BDRV_O_NO_BACKING); 
> >   
> >              bs = NULL; 
> > -            ret = bdrv_open(&bs, backing_file->value.s, NULL, NULL,  
> back_flags, 
> > +            ret = bdrv_open(&bs, backing_file, NULL, NULL, back_flags, 
> >                              backing_drv, &local_err); 
> >              if (ret < 0) { 
> >                  error_setg_errno(errp, -ret, "Could not open '%s': %s", 
> > -                                 backing_file->value.s, 
> > +                                 backing_file, 
> >                                   error_get_pretty(local_err)); 
> >                  error_free(local_err); 
> >                  local_err = NULL; 
> > @@ -5414,7 +5451,7 @@ void bdrv_img_create(const char *filename, const char  
> *fmt, 
> >              size *= 512; 
> >   
> >              snprintf(buf, sizeof(buf), "%" PRId64, size); 
> > -            set_option_parameter(param, BLOCK_OPT_SIZE, buf); 
> > +            qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size); 
> >   
> >              bdrv_unref(bs); 
> >          } else { 
> > @@ -5425,16 +5462,18 @@ void bdrv_img_create(const char *filename, const  
> char *fmt, 
> >   
> >      if (!quiet) { 
> >          printf("Formatting '%s', fmt=%s ", filename, fmt); 
> > -        print_option_parameters(param); 
> > +        qemu_opts_print(opts); 
> >          puts(""); 
> >      } 
> > -    ret = bdrv_create(drv, filename, param, &local_err); 
> > + 
> > +    ret = bdrv_create(drv, filename, NULL, opts, &local_err); 
> > + 
> >      if (ret == -EFBIG) { 
> >          /* This is generally a better message than whatever the driver  
> would 
> >           * deliver (especially because of the cluster_size_hint), since  
> that 
> >           * is most probably not much different from "image too large". */ 
> >          const char *cluster_size_hint = ""; 
> > -        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) { 
> > +        if (qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE, 0)) { 
> >              cluster_size_hint = " (try using a larger cluster size)"; 
> >          } 
> >          error_setg(errp, "The image size is too large for file format  
> '%s'" 
> > @@ -5444,9 +5483,8 @@ void bdrv_img_create(const char *filename, const char  
> *fmt, 
> >      } 
> >   
> >  out: 
> > -    free_option_parameters(create_options); 
> > -    free_option_parameters(param); 
> > - 
> > +    qemu_opts_del(opts); 
> > +    qemu_opts_free(create_opts); 
> >      if (local_err) { 
> >          error_propagate(errp, local_err); 
> >      } 
> > @@ -5464,12 +5502,36 @@ void bdrv_add_before_write_notifier(BlockDriverState  
> *bs, 
> >      notifier_with_return_list_add(&bs->before_write_notifiers, notifier); 
> >  } 
> >   
> > -int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options) 
> > +int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options, 
> > +                       QemuOpts *opts) 
> >  { 
> > -    if (bs->drv->bdrv_amend_options == NULL) { 
> > +    int ret; 
> > +    assert(!(options && opts)); 
> > + 
> > +    if (!bs->drv->bdrv_amend_options && !bs->drv->bdrv_amend_options2) { 
> >          return -ENOTSUP; 
> >      } 
> > -    return bs->drv->bdrv_amend_options(bs, options); 
> > +    if (bs->drv->bdrv_amend_options2) { 
> > +        QemuOptsList *opts_list = NULL; 
> > +        if (options) { 
> > +            opts_list = params_to_opts(options); 
> > +            opts = qemu_opts_create(opts_list, NULL, 0, &error_abort); 
> > +        } 
> > +        ret = bs->drv->bdrv_amend_options2(bs, opts); 
> > +        if (options) { 
> > +            qemu_opts_del(opts); 
> > +            qemu_opts_free(opts_list); 
> > +        } 
> > +    } else { 
> > +        if (opts) { 
> > +            options = opts_to_params(opts); 
> > +        } 
> > +        ret = bs->drv->bdrv_amend_options(bs, options); 
> > +        if (opts) { 
> > +            free_option_parameters(options); 
> > +        } 
> > +    } 
> > +    return ret; 
> >  } 
> >   
> >  /* This function will be called by the bdrv_recurse_is_first_non_filter  
> method 
> > diff --git a/block/cow.c b/block/cow.c 
> > index 30deb88..26cf4a5 100644 
> > --- a/block/cow.c 
> > +++ b/block/cow.c 
> > @@ -345,7 +345,7 @@ static int cow_create(const char *filename,  
> QEMUOptionParameter *options, 
> >          options++; 
> >      } 
> >   
> > -    ret = bdrv_create_file(filename, options, &local_err); 
> > +    ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          return ret; 
> > diff --git a/block/qcow.c b/block/qcow.c 
> > index d5a7d5f..9411aef 100644 
> > --- a/block/qcow.c 
> > +++ b/block/qcow.c 
> > @@ -687,7 +687,7 @@ static int qcow_create(const char *filename,  
> QEMUOptionParameter *options, 
> >          options++; 
> >      } 
> >   
> > -    ret = bdrv_create_file(filename, options, &local_err); 
> > +    ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          return ret; 
> > diff --git a/block/qcow2.c b/block/qcow2.c 
> > index e903d97..49985e3 100644 
> > --- a/block/qcow2.c 
> > +++ b/block/qcow2.c 
> > @@ -1625,7 +1625,7 @@ static int qcow2_create2(const char *filename, int64_t  
> total_size, 
> >      Error *local_err = NULL; 
> >      int ret; 
> >   
> > -    ret = bdrv_create_file(filename, options, &local_err); 
> > +    ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          return ret; 
> > diff --git a/block/qed.c b/block/qed.c 
> > index c130e42..2982640 100644 
> > --- a/block/qed.c 
> > +++ b/block/qed.c 
> > @@ -566,7 +566,7 @@ static int qed_create(const char *filename, uint32_t  
> cluster_size, 
> >      int ret = 0; 
> >      BlockDriverState *bs; 
> >   
> > -    ret = bdrv_create_file(filename, NULL, &local_err); 
> > +    ret = bdrv_create_file(filename, NULL, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          return ret; 
> > diff --git a/block/raw_bsd.c b/block/raw_bsd.c 
> > index 01ea692..9ae5fc2 100644 
> > --- a/block/raw_bsd.c 
> > +++ b/block/raw_bsd.c 
> > @@ -145,7 +145,7 @@ static int raw_create(const char *filename,  
> QEMUOptionParameter *options, 
> >      Error *local_err = NULL; 
> >      int ret; 
> >   
> > -    ret = bdrv_create_file(filename, options, &local_err); 
> > +    ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >      if (local_err) { 
> >          error_propagate(errp, local_err); 
> >      } 
> > diff --git a/block/vhdx.c b/block/vhdx.c 
> > index 509baaf..a9fcf6b 100644 
> > --- a/block/vhdx.c 
> > +++ b/block/vhdx.c 
> > @@ -1796,7 +1796,7 @@ static int vhdx_create(const char *filename,  
> QEMUOptionParameter *options, 
> >      block_size = block_size > VHDX_BLOCK_SIZE_MAX ? VHDX_BLOCK_SIZE_MAX : 
> >                                                      block_size; 
> >   
> > -    ret = bdrv_create_file(filename, options, &local_err); 
> > +    ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          goto exit; 
> > diff --git a/block/vmdk.c b/block/vmdk.c 
> > index 06a1f9f..3802863 100644 
> > --- a/block/vmdk.c 
> > +++ b/block/vmdk.c 
> > @@ -1526,7 +1526,7 @@ static int vmdk_create_extent(const char *filename,  
> int64_t filesize, 
> >      uint32_t *gd_buf = NULL; 
> >      int gd_buf_size; 
> >   
> > -    ret = bdrv_create_file(filename, NULL, &local_err); 
> > +    ret = bdrv_create_file(filename, NULL, NULL, &local_err); 
> >      if (ret < 0) { 
> >          error_propagate(errp, local_err); 
> >          goto exit; 
> > @@ -1866,7 +1866,7 @@ static int vmdk_create(const char *filename,  
> QEMUOptionParameter *options, 
> >      if (!split && !flat) { 
> >          desc_offset = 0x200; 
> >      } else { 
> > -        ret = bdrv_create_file(filename, options, &local_err); 
> > +        ret = bdrv_create_file(filename, options, NULL, &local_err); 
> >          if (ret < 0) { 
> >              error_setg_errno(errp, -ret, "Could not create image file"); 
> >              goto exit; 
> > diff --git a/block/vvfat.c b/block/vvfat.c 
> > index c3af7ff..155fc9b 100644 
> > --- a/block/vvfat.c 
> > +++ b/block/vvfat.c 
> > @@ -2926,7 +2926,7 @@ static int enable_write_target(BDRVVVFATState *s) 
> >      set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count *  
> 512); 
> >      set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:"); 
> >   
> > -    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err); 
> > +    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, NULL,  
> &local_err); 
> >      if (ret < 0) { 
> >          qerror_report_err(local_err); 
> >          error_free(local_err); 
> > diff --git a/include/block/block.h b/include/block/block.h 
> > index c12808a..bfa2192 100644 
> > --- a/include/block/block.h 
> > +++ b/include/block/block.h 
> > @@ -177,9 +177,9 @@ BlockDriver *bdrv_find_format(const char *format_name); 
> >  BlockDriver *bdrv_find_whitelisted_format(const char *format_name, 
> >                                            bool readonly); 
> >  int bdrv_create(BlockDriver *drv, const char* filename, 
> > -    QEMUOptionParameter *options, Error **errp); 
> > +    QEMUOptionParameter *options, QemuOpts *opts, Error **errp); 
> >  int bdrv_create_file(const char* filename, QEMUOptionParameter *options, 
> > -                     Error **errp); 
> > +                     QemuOpts *opts, Error **errp); 
> >  BlockDriverState *bdrv_new(const char *device_name, Error **errp); 
> >  void bdrv_make_anon(BlockDriverState *bs); 
> >  void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); 
> > @@ -284,7 +284,8 @@ typedef enum { 
> >   
> >  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode  
> fix); 
> >   
> > -int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter  
> *options); 
> > +int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter  
> *options, 
> > +                       QemuOpts *opts); 
> >   
> >  /* external snapshots */ 
> >  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h 
> > index cd5bc73..59875db 100644 
> > --- a/include/block/block_int.h 
> > +++ b/include/block/block_int.h 
> > @@ -118,6 +118,8 @@ struct BlockDriver { 
> >      void (*bdrv_rebind)(BlockDriverState *bs); 
> >      int (*bdrv_create)(const char *filename, QEMUOptionParameter *options, 
> >                         Error **errp); 
> > +    /* FIXME: will remove the duplicate and rename back to bdrv_create  
> later */ 
> > +    int (*bdrv_create2)(const char *filename, QemuOpts *opts, Error  
> **errp); 
> >      int (*bdrv_set_key)(BlockDriverState *bs, const char *key); 
> >      int (*bdrv_make_empty)(BlockDriverState *bs); 
> >      /* aio */ 
> > @@ -217,7 +219,12 @@ struct BlockDriver { 
> >   
> >      /* List of options for creating images, terminated by name == NULL */ 
> >      QEMUOptionParameter *create_options; 
> > - 
> > +    /* FIXME: will replace create_options. 
> > +     * These two fields are mutually exclusive. At most one is non-NULL. 
> > +     * create_options should only be set with bdrv_create, and create_opts 
> > +     * should only be set with bdrv_create2. 
> > +     */ 
> > +    QemuOptsList *create_opts; 
> >   
> >      /* 
> >       * Returns 0 for completed check, -errno for internal errors. 
> > @@ -228,6 +235,10 @@ struct BlockDriver { 
> >   
> >      int (*bdrv_amend_options)(BlockDriverState *bs, 
> >          QEMUOptionParameter *options); 
> > +    /* FIXME: will remove the duplicate and rename back to 
> > +     * bdrv_amend_options later 
> > +     */ 
> > +    int (*bdrv_amend_options2)(BlockDriverState *bs, QemuOpts *opts); 
> >   
> >      void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event); 
> >   
> > diff --git a/qemu-img.c b/qemu-img.c 
> > index 968b4c8..14d3acd 100644 
> > --- a/qemu-img.c 
> > +++ b/qemu-img.c 
> > @@ -249,7 +249,7 @@ static int read_password(char *buf, int buf_size) 
> >  static int print_block_option_help(const char *filename, const char *fmt) 
> >  { 
> >      BlockDriver *drv, *proto_drv; 
> > -    QEMUOptionParameter *create_options = NULL; 
> > +    QemuOptsList *create_opts = NULL; 
> >   
> >      /* Find driver and parse its options */ 
> >      drv = bdrv_find_format(fmt); 
> > @@ -258,21 +258,20 @@ static int print_block_option_help(const char  
> *filename, const char *fmt) 
> >          return 1; 
> >      } 
> >   
> > -    create_options = append_option_parameters(create_options, 
> > -                                              drv->create_options); 
> > - 
> > +    create_opts = qemu_opts_append(create_opts, drv->create_opts, 
> > +                                   drv->create_options); 
> >      if (filename) { 
> >          proto_drv = bdrv_find_protocol(filename, true); 
> >          if (!proto_drv) { 
> >              error_report("Unknown protocol '%s'", filename); 
> >              return 1; 
> >          } 
> > -        create_options = append_option_parameters(create_options, 
> > -                                                  proto_drv->create_options); 
> > +        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, 
> > +                                       proto_drv->create_options); 
> >      } 
> >   
> > -    print_option_help(create_options); 
> > -    free_option_parameters(create_options); 
> > +    qemu_opts_print_help(create_opts); 
> > +    qemu_opts_free(create_opts); 
> >      return 0; 
> >  } 
> >   
> > @@ -326,19 +325,19 @@ fail: 
> >      return NULL; 
> >  } 
> >   
> > -static int add_old_style_options(const char *fmt, QEMUOptionParameter  
> *list, 
> > +static int add_old_style_options(const char *fmt, QemuOpts *opts, 
> >                                   const char *base_filename, 
> >                                   const char *base_fmt) 
> >  { 
> >      if (base_filename) { 
> > -        if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE,  
> base_filename)) { 
> > +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename)) { 
> >              error_report("Backing file not supported for file format  
> '%s'", 
> >                           fmt); 
> >              return -1; 
> >          } 
> >      } 
> >      if (base_fmt) { 
> > -        if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) { 
> > +        if (qemu_opt_set(opts, BLOCK_OPT_BACKING_FMT, base_fmt)) { 
> >              error_report("Backing file format not supported for file " 
> >                           "format '%s'", fmt); 
> >              return -1; 
> > @@ -1169,8 +1168,9 @@ static int img_convert(int argc, char **argv) 
> >      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE; 
> >      const uint8_t *buf1; 
> >      BlockDriverInfo bdi; 
> > -    QEMUOptionParameter *param = NULL, *create_options = NULL; 
> > -    QEMUOptionParameter *out_baseimg_param; 
> > +    QemuOpts *opts = NULL; 
> > +    QemuOptsList *create_opts = NULL; 
> > +    const char *out_baseimg_param; 
> >      char *options = NULL; 
> >      const char *snapshot_name = NULL; 
> >      int min_sparse = 8; /* Need at least 4k of zeros for sparse detection  
> */ 
> > @@ -1359,40 +1359,36 @@ static int img_convert(int argc, char **argv) 
> >          goto out; 
> >      } 
> >   
> > -    create_options = append_option_parameters(create_options, 
> > -                                              drv->create_options); 
> > -    create_options = append_option_parameters(create_options, 
> > -                                              proto_drv->create_options); 
> > +    create_opts = qemu_opts_append(create_opts, drv->create_opts, 
> > +                                   drv->create_options); 
> > +    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts, 
> > +                                   proto_drv->create_options); 
> >   
> > -    if (options) { 
> > -        param = parse_option_parameters(options, create_options, param); 
> > -        if (param == NULL) { 
> > -            error_report("Invalid options for file format '%s'.", out_fmt); 
> > -            ret = -1; 
> > -            goto out; 
> > -        } 
> > -    } else { 
> > -        param = parse_option_parameters("", create_options, param); 
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); 
> > +    if (options && qemu_opts_do_parse(opts, options, NULL)) { 
> > +        error_report("Invalid options for file format '%s'.", out_fmt); 
> > +        ret = -1; 
> > +        goto out; 
> >      } 
> >   
> > -    set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512); 
> > -    ret = add_old_style_options(out_fmt, param, out_baseimg, NULL); 
> > +    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, total_sectors * 512); 
> > +    ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL); 
> >      if (ret < 0) { 
> >          goto out; 
> >      } 
> >   
> >      /* Get backing file name if -o backing_file was used */ 
> > -    out_baseimg_param = get_option_parameter(param,  
> BLOCK_OPT_BACKING_FILE); 
> > +    out_baseimg_param = qemu_opt_get(opts, BLOCK_OPT_BACKING_FILE); 
> >      if (out_baseimg_param) { 
> > -        out_baseimg = out_baseimg_param->value.s; 
> > +        out_baseimg = out_baseimg_param; 
> >      } 
> >   
> >      /* Check if compression is supported */ 
> >      if (compress) { 
> > -        QEMUOptionParameter *encryption = 
> > -            get_option_parameter(param, BLOCK_OPT_ENCRYPT); 
> > -        QEMUOptionParameter *preallocation = 
> > -            get_option_parameter(param, BLOCK_OPT_PREALLOC); 
> > +        bool encryption = 
> > +            qemu_opt_get_bool(opts, BLOCK_OPT_ENCRYPT, false); 
> > +        const char *preallocation = 
> > +            qemu_opt_get(opts, BLOCK_OPT_PREALLOC); 
> >   
> >          if (!drv->bdrv_write_compressed) { 
> >              error_report("Compression not supported for this file  
> format"); 
> > @@ -1400,15 +1396,15 @@ static int img_convert(int argc, char **argv) 
> >              goto out; 
> >          } 
> >   
> > -        if (encryption && encryption->value.n) { 
> > +        if (encryption) { 
> >              error_report("Compression and encryption not supported at " 
> >                           "the same time"); 
> >              ret = -1; 
> >              goto out; 
> >          } 
> >   
> > -        if (preallocation && preallocation->value.s 
> > -            && strcmp(preallocation->value.s, "off")) 
> > +        if (preallocation 
> > +            && strcmp(preallocation, "off")) 
> >          { 
> >              error_report("Compression and preallocation not supported at " 
> >                           "the same time"); 
> > @@ -1419,7 +1415,7 @@ static int img_convert(int argc, char **argv) 
> >   
> >      if (!skip_create) { 
> >          /* Create the new image */ 
> > -        ret = bdrv_create(drv, out_filename, param, &local_err); 
> > +        ret = bdrv_create(drv, out_filename, NULL, opts, &local_err); 
> >          if (ret < 0) { 
> >              error_report("%s: error while converting %s: %s", 
> >                           out_filename, out_fmt,  
> error_get_pretty(local_err)); 
> > @@ -1683,8 +1679,8 @@ out: 
> >          qemu_progress_print(100, 0); 
> >      } 
> >      qemu_progress_end(); 
> > -    free_option_parameters(create_options); 
> > -    free_option_parameters(param); 
> > +    qemu_opts_del(opts); 
> > +    qemu_opts_free(create_opts); 
> >      qemu_vfree(buf); 
> >      if (sn_opts) { 
> >          qemu_opts_del(sn_opts); 
> > @@ -2675,7 +2671,8 @@ static int img_amend(int argc, char **argv) 
> >  { 
> >      int c, ret = 0; 
> >      char *options = NULL; 
> > -    QEMUOptionParameter *create_options = NULL, *options_param = NULL; 
> > +    QemuOptsList *create_opts = NULL; 
> > +    QemuOpts *opts = NULL; 
> >      const char *fmt = NULL, *filename; 
> >      bool quiet = false; 
> >      BlockDriverState *bs = NULL; 
> > @@ -2746,17 +2743,16 @@ static int img_amend(int argc, char **argv) 
> >          goto out; 
> >      } 
> >   
> > -    create_options = append_option_parameters(create_options, 
> > -            bs->drv->create_options); 
> > -    options_param = parse_option_parameters(options, create_options, 
> > -            options_param); 
> > -    if (options_param == NULL) { 
> > +    create_opts = qemu_opts_append(create_opts, bs->drv->create_opts, 
> > +                                   bs->drv->create_options); 
> > +    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort); 
> > +    if (options && qemu_opts_do_parse(opts, options, NULL)) { 
>  
>  
> Since now we're calling qemu_opts_do_parse() instead of  
> parse_option_parameters() 
> you should update the 061 test output, otherwise the test will fail due to  
> wrong 
> output, the following should be enough: 
>  
> --- a/tests/qemu-iotests/061.out 
> +++ b/tests/qemu-iotests/061.out 
> @@ -281,7 +281,7 @@ Lazy refcounts only supported with compatibility level  
> 1.1 and above (use compat 
>  qemu-img: Error while amending options: Invalid argument 
>  Unknown compatibility level 0.42. 
>  qemu-img: Error while amending options: Invalid argument 
> -Unknown option 'foo' 
> +qemu-img: Invalid parameter 'foo' 
>  qemu-img: Invalid options for file format 'qcow2' 
>  Changing the cluster size is not supported. 
>  qemu-img: Error while amending options: Operation not supported 
>  
> Regards...