Re: [Qemu-devel] [PATCH V18 04/25] qemu-option: create some QemuOpts functons

[Kevin Wolf <kwolf@redhat.com>]

Am 13.08.2013 um 06:31 hat Dong Xu Wang geschrieben:
> These functions will be used in next commit.
> 
> qemu_opt_get_(*)_del functions are used to make sure we
> have the same behaviors as before: in block layer, after
> parsing a parameter value, parameter list will delete it
> to avoid parsing it twice.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> 
> v16->v17:
> 1) return "char *" instead of "const char*"
> 
> v13->v14:
> 1) rewrite commit message.
> 2) use def_value_str in qemu_opt_get_FOO_del() and qemu_opt_get_del().
> 3) delete redundant qemu_opt_del(opt).
> ---
>  include/qemu/option.h |  11 ++++-
>  util/qemu-option.c    | 130 ++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 125 insertions(+), 16 deletions(-)
> 
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 3b514bb..64d7327 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -108,6 +108,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
> @@ -121,14 +122,19 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name);
>   */
>  bool qemu_opt_has_help_opt(QemuOpts *opts);
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
> +bool qemu_opt_get_bool_del(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);
>  int qemu_opt_unset(QemuOpts *opts, const char *name);
> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
> +                               uint64_t defval);
>  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value);
>  void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>                        Error **errp);
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
>  int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val);
> +int qemu_opt_replace_set_number(QemuOpts *opts, const char *name, int64_t val);
>  typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
>  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>                       int abort_on_failure);
> @@ -145,7 +151,10 @@ const char *qemu_opts_id(QemuOpts *opts);
>  void qemu_opts_del(QemuOpts *opts);
>  void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
>  int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
> +                               const char *firstname);
> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
> +                          int permit_abbrev);
>  void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>                              int permit_abbrev);
>  QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 2a5447c..b939e8a 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -523,6 +523,32 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>      return opt ? opt->str : NULL;
>  }
>  
> +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);
> +}
> +
> +char *qemu_opt_get_del(QemuOpts *opts, const char *name)
> +{
> +    QemuOpt *opt = qemu_opt_find(opts, name);
> +    const QemuOptDesc *desc;
> +    char *str = NULL;
> +
> +    if (!opt) {
> +        desc = find_desc_by_name(opts->list->desc, name);
> +        if (desc && desc->def_value_str) {
> +            str = g_strdup(desc->def_value_str);
> +        }
> +    } else {
> +        str = g_strdup(opt->str);
> +        qemu_opt_del(opt);
> +    }
> +    return str;
> +}

This is almost duplicating qemu_opt_get(). Can't it be a thin wrapper
around the original function?

Also, any reason to g_strdup() here when the original function returns a
reference to the existing string? This makes the interface inconsistent.

Same comments apply to qemu_opt_get_bool_del() and
qemu_opt_get_size_del().

Kevin