| Message ID | 1397152467-17186-7-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On 04/10/2014 11:54 AM, Chunyan Liu wrote: > Add qemu_opt_get_del, qemu_opt_get_bool_del, qemu_opt_get_number_del and > qemu_opt_get_size_del to replace the same handling of QEMUOptionParamter s/Paramter/Parameter/ > (get and delete). > > Several drivers are coded to parse a known subset of options, then > remove them from the list before handing all remaining options to a > second driver for further option processing. get_*_del makes it easier > to retrieve a known option (or its default) and remove it from the list > all in one action. > > Share common helper function: > > For qemu_opt_get_bool/size/number, they and their get_*_del counterpart > could share most of the code except whether or not deleting the opt from > option list, so generate common helper functions. > > For qemu_opt_get and qemu_opt_get_del, keep code duplication, since > 1. qemu_opt_get_del returns malloc'd memory while qemu_opt_get returns > in-place memory > 2. qemu_opt_get_del returns (char *), qemu_opt_get returns (const char *), > and could not change to (char *), since in one case, it will return > desc->def_value_str, which is (const char *). > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > include/qemu/option.h | 6 +++ > util/qemu-option.c | 116 ++++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 109 insertions(+), 13 deletions(-) > > +++ b/util/qemu-option.c > @@ -575,6 +575,19 @@ static void qemu_opt_del(QemuOpt *opt) > g_free(opt); > } > > +/* qemu_opt_set allow many settings for the same option. s/allow/allows/ Question for a possible future patch: who relies on qemu_opt_set supporting multiple settings for the same option? Would it make the code any cleaner to rework qemu_opt_set to track only one value for an option, with the caller having the option of either flagging duplicates as an error or silently replacing the old option value with the new? But that doesn't belong in this patch, so it doesn't impact my review. > + * This function is to delete all settings for an option. s/is to delete/deletes/ > +/* Get a known option (or its default) and remove it from the list > + * all in one action. Return a malloced string of the option vaule. s/vaule/value/ As my findings on this patch are limited to typo fixes, it is trivial enough that you can fix them and add: Reviewed-by: Eric Blake <eblake@redhat.com>
>>> On 4/22/2014 at 03:11 AM, in message <53556D4C.5070906@redhat.com>, Eric Blake <eblake@redhat.com> wrote: > On 04/10/2014 11:54 AM, Chunyan Liu wrote: > > Add qemu_opt_get_del, qemu_opt_get_bool_del, qemu_opt_get_number_del and > > qemu_opt_get_size_del to replace the same handling of QEMUOptionParamter > > s/Paramter/Parameter/ > > > (get and delete). > > > > Several drivers are coded to parse a known subset of options, then > > remove them from the list before handing all remaining options to a > > second driver for further option processing. get_*_del makes it easier > > to retrieve a known option (or its default) and remove it from the list > > all in one action. > > > > Share common helper function: > > > > For qemu_opt_get_bool/size/number, they and their get_*_del counterpart > > could share most of the code except whether or not deleting the opt from > > option list, so generate common helper functions. > > > > For qemu_opt_get and qemu_opt_get_del, keep code duplication, since > > 1. qemu_opt_get_del returns malloc'd memory while qemu_opt_get returns > > in-place memory > > 2. qemu_opt_get_del returns (char *), qemu_opt_get returns (const char *), > > and could not change to (char *), since in one case, it will return > > desc->def_value_str, which is (const char *). > > > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > include/qemu/option.h | 6 +++ > > util/qemu-option.c | 116 > ++++++++++++++++++++++++++++++++++++++++++++------ > > 2 files changed, 109 insertions(+), 13 deletions(-) > > > > > +++ b/util/qemu-option.c > > @@ -575,6 +575,19 @@ static void qemu_opt_del(QemuOpt *opt) > > g_free(opt); > > } > > > > +/* qemu_opt_set allow many settings for the same option. > > s/allow/allows/ > > Question for a possible future patch: who relies on qemu_opt_set > supporting multiple settings for the same option? Would it make the I checked the commit log, it changed into "Never overwrite a QemuOpt" in this commit for cases like "-net user,hostfwd=" : dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9 Not sure if it is stilled used. > code any cleaner to rework qemu_opt_set to track only one value for an > option, with the caller having the option of either flagging duplicates > as an error or silently replacing the old option value with the new? > But that doesn't belong in this patch, so it doesn't impact my review. > > > + * This function is to delete all settings for an option. > > s/is to delete/deletes/ > > > +/* Get a known option (or its default) and remove it from the list > > + * all in one action. Return a malloced string of the option vaule. > > s/vaule/value/ > > As my findings on this patch are limited to typo fixes, it is trivial > enough that you can fix them and add: > Reviewed-by: Eric Blake <eblake@redhat.com> > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
diff --git a/include/qemu/option.h b/include/qemu/option.h index c3b0a91..6653e43 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -111,6 +111,7 @@ struct QemuOptsList { }; const char *qemu_opt_get(QemuOpts *opts, const char *name); +char *qemu_opt_get_del(QemuOpts *opts, const char *name); /** * qemu_opt_has_help_opt: * @opts: options to search for a help request @@ -126,6 +127,11 @@ bool qemu_opt_has_help_opt(QemuOpts *opts); 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); uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval); +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval); +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name, + uint64_t defval); +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name, + uint64_t defval); int qemu_opt_unset(QemuOpts *opts, const char *name); int qemu_opt_set(QemuOpts *opts, const char *name, const char *value); void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value, diff --git a/util/qemu-option.c b/util/qemu-option.c index 9f59c07..70743a4 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -575,6 +575,19 @@ static void qemu_opt_del(QemuOpt *opt) g_free(opt); } +/* qemu_opt_set allow many settings for the same option. + * This function is to delete all settings for an option. + */ +static void qemu_opt_del_all(QemuOpts *opts, const char *name) +{ + QemuOpt *opt, *next_opt; + + QTAILQ_FOREACH_SAFE(opt, &opts->head, next, next_opt) { + if (!strcmp(opt->name, name)) + qemu_opt_del(opt); + } +} + const char *qemu_opt_get(QemuOpts *opts, const char *name) { QemuOpt *opt = qemu_opt_find(opts, name); @@ -588,6 +601,34 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name) return opt ? opt->str : NULL; } +/* Get a known option (or its default) and remove it from the list + * all in one action. Return a malloced string of the option vaule. + * Result must be freed by caller with g_free(). + */ +char *qemu_opt_get_del(QemuOpts *opts, const char *name) +{ + QemuOpt *opt; + const QemuOptDesc *desc; + char *str = NULL; + + if (opts == NULL) { + 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) { + str = g_strdup(desc->def_value_str); + } + return str; + } + str = opt->str; + opt->str = NULL; + qemu_opt_del_all(opts, name); + return str; +} + bool qemu_opt_has_help_opt(QemuOpts *opts) { QemuOpt *opt; @@ -600,50 +641,99 @@ bool qemu_opt_has_help_opt(QemuOpts *opts) return false; } -bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) +static bool qemu_opt_get_bool_helper(QemuOpts *opts, const char *name, + bool defval, bool del) { QemuOpt *opt = qemu_opt_find(opts, name); + bool ret = defval; if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { - parse_option_bool(name, desc->def_value_str, &defval, &error_abort); + parse_option_bool(name, desc->def_value_str, &ret, &error_abort); } - return defval; + return ret; } assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL); - return opt->value.boolean; + ret = opt->value.boolean; + if (del) { + qemu_opt_del_all(opts, name); + } + return ret; } -uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) +bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) +{ + return qemu_opt_get_bool_helper(opts, name, defval, false); +} + +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval) +{ + return qemu_opt_get_bool_helper(opts, name, defval, true); +} + +static uint64_t qemu_opt_get_number_helper(QemuOpts *opts, const char *name, + uint64_t defval, bool del) { QemuOpt *opt = qemu_opt_find(opts, name); + uint64_t ret = defval; if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { - parse_option_number(name, desc->def_value_str, &defval, - &error_abort); + parse_option_number(name, desc->def_value_str, &ret, &error_abort); } - return defval; + return ret; } assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER); - return opt->value.uint; + ret = opt->value.uint; + if (del) { + qemu_opt_del_all(opts, name); + } + return ret; } -uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) +uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) +{ + return qemu_opt_get_number_helper(opts, name, defval, false); +} + +uint64_t qemu_opt_get_number_del(QemuOpts *opts, const char *name, + uint64_t defval) +{ + return qemu_opt_get_number_helper(opts, name, defval, true); +} + +static uint64_t qemu_opt_get_size_helper(QemuOpts *opts, const char *name, + uint64_t defval, bool del) { QemuOpt *opt = qemu_opt_find(opts, name); + uint64_t ret = defval; if (opt == NULL) { const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); if (desc && desc->def_value_str) { - parse_option_size(name, desc->def_value_str, &defval, &error_abort); + parse_option_size(name, desc->def_value_str, &ret, &error_abort); } - return defval; + return ret; } assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE); - return opt->value.uint; + ret = opt->value.uint; + if (del) { + qemu_opt_del_all(opts, name); + } + return ret; +} + +uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) +{ + return qemu_opt_get_size_helper(opts, name, defval, false); +} + +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name, + uint64_t defval) +{ + return qemu_opt_get_size_helper(opts, name, defval, true); } static void qemu_opt_parse(QemuOpt *opt, Error **errp)