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