| Message ID | 1392186806-10418-4-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On 02/11/2014 11:33 PM, 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 s/NUlL/NULL/ > with new value. > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > util/qemu-option.c | 84 +++++++++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 70 insertions(+), 14 deletions(-) > > diff --git a/util/qemu-option.c b/util/qemu-option.c > index fd84f95..ea6793a 100644 > --- a/util/qemu-option.c > +++ b/util/qemu-option.c > @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name) > { > QemuOpt *opt; > > + if (!opts) > + return NULL; > + Why not just 'assert(opts);'? In other words, what caller is planning on passing a NULL opts, since I couldn't find any existing caller that did that? Please update the commit message with justification of how this can simplify a caller in a later patch, if that is your plan; but without that justification, I'm going solely off the diffstat and the fact that no existing caller passes NULL, and concluding that this change is bloat. > + opt = qemu_opt_find(opts, name); > if (!opt) { > + > + opt = qemu_opt_find(opts, name); > + > if (opt == NULL) { Inconsistent styles here; might be worth making them all look alike while touching them, if we keep this patch. > @@ -664,6 +693,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); Ugg. Why did the struct declare things as const char *str if we are going to be strdup'ing into it? I worry about attempting to free non-malloc'd memory any time I see a cast to lose a const. Is this just a case of someone incorrectly trying to be const-correct, and now you have to work around it? If you drop the 'const' from option_int.h, do things still compile?
2014-02-13 7:22 GMT+08:00 Eric Blake <eblake@redhat.com>: > On 02/11/2014 11:33 PM, 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 > > s/NUlL/NULL/ > > > with new value. > > > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > util/qemu-option.c | 84 > +++++++++++++++++++++++++++++++++++++++++++-------- > > 1 files changed, 70 insertions(+), 14 deletions(-) > > > > diff --git a/util/qemu-option.c b/util/qemu-option.c > > index fd84f95..ea6793a 100644 > > --- a/util/qemu-option.c > > +++ b/util/qemu-option.c > > @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const > char *name) > > { > > QemuOpt *opt; > > > > + if (!opts) > > + return NULL; > > + > > Why not just 'assert(opts);'? In other words, what caller is planning > on passing a NULL opts, since I couldn't find any existing caller that > did that? Please update the commit message with justification of how > this can simplify a caller in a later patch, if that is your plan; but > without that justification, I'm going solely off the diffstat and the > fact that no existing caller passes NULL, and concluding that this > change is bloat. > > In existing code, qemu_opt_get* and qemu_opt_unset are calling it, both directly call qemu_opt_find without checking opts==NULL case. For these patch series, after adding opts check in qemu_opt_get, I can remove that from qemu_opt_find, no problem. I just thought it would be safer to add a check. > > + opt = qemu_opt_find(opts, name); > > if (!opt) { > > > + > > + opt = qemu_opt_find(opts, name); > > + > > if (opt == NULL) { > > Inconsistent styles here; might be worth making them all look alike > while touching them, if we keep this patch. > > > @@ -664,6 +693,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); > > Ugg. Why did the struct declare things as const char *str if we are > going to be strdup'ing into it? I worry about attempting to free > non-malloc'd memory any time I see a cast to lose a const. Is this just > a case of someone incorrectly trying to be const-correct, and now you > have to work around it? If you drop the 'const' from option_int.h, do > things still compile? > > I followed existing code (incorrectly using). Will remove "const" (still compile). Thanks. > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
diff --git a/util/qemu-option.c b/util/qemu-option.c index fd84f95..ea6793a 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name) { QemuOpt *opt; + if (!opts) + return NULL; + QTAILQ_FOREACH_REVERSE(opt, &opts->head, QemuOptHead, next) { if (strcmp(opt->name, name) != 0) continue; @@ -509,9 +512,14 @@ 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; const QemuOptDesc *desc; + if (!opts) { + return NULL; + } + + opt = qemu_opt_find(opts, name); if (!opt) { desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -535,10 +543,16 @@ 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; const QemuOptDesc *desc; Error *local_err = NULL; + if (!opts) { + return defval; + } + + opt = qemu_opt_find(opts, name); + if (opt == NULL) { desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -553,10 +567,16 @@ 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; const QemuOptDesc *desc; Error *local_err = NULL; + if (!opts) { + return defval; + } + + opt = qemu_opt_find(opts, name); + if (opt == NULL) { desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -571,10 +591,15 @@ 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; const QemuOptDesc *desc; Error *local_err = NULL; + if (!opts) { + return defval; + } + + opt = qemu_opt_find(opts, name); if (opt == NULL) { desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { @@ -612,6 +637,10 @@ static void qemu_opt_parse(QemuOpt *opt, Error **errp) static void qemu_opt_del(QemuOpt *opt) { + if (!opt) { + return; + } + QTAILQ_REMOVE(&opt->opts->head, opt, next); g_free((/* !const */ char*)opt->name); g_free((/* !const */ char*)opt->str); @@ -664,6 +693,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; @@ -704,16 +740,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; @@ -726,16 +770,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; @@ -870,6 +922,10 @@ void qemu_opts_del(QemuOpts *opts) { QemuOpt *opt; + if (!opts) { + return; + } + for (;;) { opt = QTAILQ_FIRST(&opts->head); if (opt == NULL)