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

[Chunyan Liu]

2014-03-18 3:58 GMT+08:00 Leandro Dorileo <address@hidden>:

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?

It should be broken in generating patches (so I thought we should add input check), although
might not be broken at the end :) Maybe I could look at and try to exclude some changes

that's not mandatory to just make this patch series work. Like: don't need to change assertion
as in patch 4/25, don't need to replace existing setting in qemu_opt_set_* functions as in

patch 3/25.

 

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

Could do. Either change qemu_opt_find or qemu_opt_get.
I can update to change qemu_opt_find :)

 

--
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
>
>