2014-03-18 3:58 GMT+08:00 Leandro Dorileo <[email protected]>:
> 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 <[email protected]>
> > Signed-off-by: Chunyan Liu <[email protected]>
> > ---
> > 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
> >
> >
>
>
