2014-03-18 3:58 GMT+08:00 Leandro Dorileo <l...@dorileo.org>:

> 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 <wdon...@linux.vnet.ibm.com>
> > Signed-off-by: Chunyan Liu <cy...@suse.com>
> > ---
> >  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
> >
> >
>
>

Reply via email to