Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c

[Leandro Dorileo]

Hi,

On Mon, Mar 10, 2014 at 03:31:39PM +0800, Chunyan Liu wrote:
> Improve opt_get and opt_set group of functions. For opt_get, check and handle
> NULL input; for opt_set, when set to an existing option, rewrite the option
> with new value.
> 
> Signed-off-by: Dong Xu Wang <address@hidden>
> Signed-off-by: Chunyan Liu <address@hidden>
> ---
>  include/qemu/option_int.h |  4 +--
>  util/qemu-option.c        | 81 
> +++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
> index 8212fa4..db9ed91 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/util/qemu-option.c b/util/qemu-option.c
> index 65d1c22..c7639e8 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -558,8 +558,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);
> +    QemuOpt *opt;
>  
> +    if (opts == NULL) {
> +        return NULL;
> +    }



I understand you want to prevent a segfault in qemu_opt_find(), and I'm fine 
with that,
but these checks make me wonder why you decided to change it, aren't you hiding 
some
broken caller?

Btw, you could change qemu_opt_find() and check if opts != NULL there and not 
introduce
all these opts == NULL qemu_opt_find() pairs.

--
Dorileo

> +
> +    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) {
> @@ -583,7 +588,13 @@ bool qemu_opt_has_help_opt(QemuOpts *opts)
>  
>  bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
> +
> +    if (opts == NULL) {
> +        return defval;
> +    }
> +
> +    opt = qemu_opt_find(opts, name);
>  
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> @@ -598,7 +609,13 @@ 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)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
> +
> +    if (opts == NULL) {
> +        return defval;
> +    }
> +
> +    opt = qemu_opt_find(opts, name);
>  
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
> @@ -614,8 +631,13 @@ 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)
>  {
> -    QemuOpt *opt = qemu_opt_find(opts, name);
> +    QemuOpt *opt;
>  
> +    if (opts == NULL) {
> +        return defval;
> +    }
> +
> +    opt = qemu_opt_find(opts, name);
>      if (opt == NULL) {
>          const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name);
>          if (desc && desc->def_value_str) {
> @@ -652,6 +674,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>  
>  static void qemu_opt_del(QemuOpt *opt)
>  {
> +    if (opt == NULL) {
> +        return;
> +    }
> +
>      QTAILQ_REMOVE(&opt->opts->head, opt, next);
>      g_free((/* !const */ char*)opt->name);
>      g_free((/* !const */ char*)opt->str);
> @@ -704,6 +730,13 @@ static void opt_set(QemuOpts *opts, const char *name, 
> const char *value,
>          return;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);
> +        opt->str = g_strdup(value);
> +        return;
> +    }
> +
>      opt = g_malloc0(sizeof(*opt));
>      opt->name = g_strdup(name);
>      opt->opts = opts;
> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, 
> const char *value,
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>  {
>      QemuOpt *opt;
> -    const QemuOptDesc *desc = opts->list->desc;
> +    const QemuOptDesc *desc;
>  
> -    opt = g_malloc0(sizeof(*opt));
> -    opt->desc = find_desc_by_name(desc, name);
> -    if (!opt->desc && !opts_accepts_any(opts)) {
> +    desc = find_desc_by_name(opts->list->desc, name);
> +    if (!desc && !opts_accepts_any(opts)) {
>          qerror_report(QERR_INVALID_PARAMETER, name);
> -        g_free(opt);
>          return -1;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);
> +        opt->value.boolean = val;
> +        opt->str = g_strdup(val ? "on" : "off");
> +        return 0;
> +    }
> +
> +    opt = g_malloc0(sizeof(*opt));
> +    opt->desc = desc;
>      opt->name = g_strdup(name);
>      opt->opts = opts;
>      opt->value.boolean = !!val;
> @@ -766,16 +807,24 @@ 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)
>  {
>      QemuOpt *opt;
> -    const QemuOptDesc *desc = opts->list->desc;
> +    const QemuOptDesc *desc;
>  
> -    opt = g_malloc0(sizeof(*opt));
> -    opt->desc = find_desc_by_name(desc, name);
> -    if (!opt->desc && !opts_accepts_any(opts)) {
> +    desc = find_desc_by_name(opts->list->desc, name);
> +    if (!desc && !opts_accepts_any(opts)) {
>          qerror_report(QERR_INVALID_PARAMETER, name);
> -        g_free(opt);
>          return -1;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);
> +        opt->value.uint = val;
> +        opt->str = g_strdup_printf("%" PRId64, val);
> +        return 0;
> +    }
> +
> +    opt = g_malloc0(sizeof(*opt));
> +    opt->desc = desc;
>      opt->name = g_strdup(name);
>      opt->opts = opts;
>      opt->value.uint = val;
> @@ -910,6 +959,10 @@ void qemu_opts_del(QemuOpts *opts)
>  {
>      QemuOpt *opt;
>  
> +    if (opts == NULL) {
> +        return;
> +    }
> +
>      for (;;) {
>          opt = QTAILQ_FIRST(&opts->head);
>          if (opt == NULL)
> -- 
> 1.7.12.4
> 
>