| Message ID | 1397152467-17186-3-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
On 04/10/2014 11:53 AM, Chunyan Liu wrote: > Add def_value_str (default value) to QemuOptDesc, to replace function of the > default value in QEMUOptionParameter. > > Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise, > if desc->def_value_str is set, return desc->def_value_str; otherwise, return > input defval. > > Improve qemu_opts_print: if option is set, print opt->str; otherwise, if > desc->def_value_str is set, also print it. It will replace > print_option_parameters. To avoid print info changes, change qemu_opts_print > from fprintf stderr to printf, and remove last printf. This still feels like a bunch to be squashing into one patch. Two possibilities that would have made it nicer to review (either one could be done in isolation, or even doing both if you have a reason to respin): 1. Clearly document in the commit message that there are NO current callers of qemu_opts_print - it is dead code in this patch but later in the series will make use of it, so we are free to change it however we'd like to make it useful when it is no longer dead code 2. This is a lot of change in one patch. Splitting into two parts (repurpose qemu_opts_print, but without default value handling; then add default handling along with the change toe qemu_opts_print to print a default) splits the functionality of the two patches But as this series is already gone through so many revisions, and was small enough for me to look at again... > > Reviewed-by: Leandro Dorileo <l@dorileo.org> > Reviewed-by: Eric Blake <eblake@redhat.com> ...I'm fine with keeping my review. > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > changes: > * Following Leandro's comment: > merge v24 0002-QemuOpts-add-def_value_str-to-QemuOptDesc.patch and > v24 0011-qemu_opts_print-change-fprintf-stderr-to-printf.patch into one. Normally, when you make non-trivial changes based on a previous review, it is wise to drop the Reviewed-by for anyone that you want to re-review those changes.
>>> On 4/22/2014 at 02:22 AM, in message <535561E2.3000405@redhat.com>, Eric Blake <eblake@redhat.com> wrote: > On 04/10/2014 11:53 AM, Chunyan Liu wrote: > > Add def_value_str (default value) to QemuOptDesc, to replace function of > the > > default value in QEMUOptionParameter. > > > > Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise, > > if desc->def_value_str is set, return desc->def_value_str; otherwise, return > > input defval. > > > > Improve qemu_opts_print: if option is set, print opt->str; otherwise, if > > desc->def_value_str is set, also print it. It will replace > > print_option_parameters. To avoid print info changes, change > qemu_opts_print > > from fprintf stderr to printf, and remove last printf. > > This still feels like a bunch to be squashing into one patch. Two > possibilities that would have made it nicer to review (either one could > be done in isolation, or even doing both if you have a reason to respin): > > 1. Clearly document in the commit message that there are NO current > callers of qemu_opts_print - it is dead code in this patch but later in > the series will make use of it, so we are free to change it however we'd > like to make it useful when it is no longer dead code > > 2. This is a lot of change in one patch. Splitting into two parts > (repurpose qemu_opts_print, but without default value handling; then add > default handling along with the change toe qemu_opts_print to print a > default) splits the functionality of the two patches OK. I'll split it into two parts as in v24, and improve description: patch 1 (print default value, same as v22) patch2 (repurpose qemu_opts_print, to replace print_options function, as in v24). > > But as this series is already gone through so many revisions, and was > small enough for me to look at again... > > > > > Reviewed-by: Leandro Dorileo <l@dorileo.org> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > ...I'm fine with keeping my review. > > > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cyliu@suse.com> > > --- > > changes: > > * Following Leandro's comment: > > merge v24 0002-QemuOpts-add-def_value_str-to-QemuOptDesc.patch and > > v24 0011-qemu_opts_print-change-fprintf-stderr-to-printf.patch into > one. > > Normally, when you make non-trivial changes based on a previous review, > it is wise to drop the Reviewed-by for anyone that you want to re-review > those changes. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
On Fri, Apr 11, 2014 at 01:53:58AM +0800, Chunyan Liu wrote: > Add def_value_str (default value) to QemuOptDesc, to replace function of the > default value in QEMUOptionParameter. > > Improve qemu_opts_get_* functions: if find opt, return opt->str; otherwise, > if desc->def_value_str is set, return desc->def_value_str; otherwise, return > input defval. > > Improve qemu_opts_print: if option is set, print opt->str; otherwise, if > desc->def_value_str is set, also print it. It will replace > print_option_parameters. To avoid print info changes, change qemu_opts_print > from fprintf stderr to printf, and remove last printf. > > Reviewed-by: Leandro Dorileo <l@dorileo.org> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com> > Signed-off-by: Chunyan Liu <cyliu@suse.com> > --- > changes: > * Following Leandro's comment: > merge v24 0002-QemuOpts-add-def_value_str-to-QemuOptDesc.patch and > v24 0011-qemu_opts_print-change-fprintf-stderr-to-printf.patch into one. > > include/qemu/option.h | 3 ++- > util/qemu-option.c | 60 ++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 52 insertions(+), 11 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/include/qemu/option.h b/include/qemu/option.h index 8c0ac34..c3b0a91 100644 --- a/include/qemu/option.h +++ b/include/qemu/option.h @@ -99,6 +99,7 @@ typedef struct QemuOptDesc { const char *name; enum QemuOptType type; const char *help; + const char *def_value_str; } QemuOptDesc; struct QemuOptsList { @@ -156,7 +157,7 @@ QDict *qemu_opts_to_qdict(QemuOpts *opts, QDict *qdict); void qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, Error **errp); typedef int (*qemu_opts_loopfunc)(QemuOpts *opts, void *opaque); -int qemu_opts_print(QemuOpts *opts, void *dummy); +void qemu_opts_print(QemuOpts *opts); int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque, int abort_on_failure); diff --git a/util/qemu-option.c b/util/qemu-option.c index e6d10bc..84f0d5c 100644 --- a/util/qemu-option.c +++ b/util/qemu-option.c @@ -570,6 +570,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); + + if (!opt) { + const QemuOptDesc *desc = find_desc_by_name(opts->list->desc, name); + if (desc && desc->def_value_str) { + return desc->def_value_str; + } + } return opt ? opt->str : NULL; } @@ -589,8 +596,13 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval) { QemuOpt *opt = qemu_opt_find(opts, name); - if (opt == NULL) + 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); + } return defval; + } assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL); return opt->value.boolean; } @@ -599,8 +611,14 @@ uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval) { QemuOpt *opt = qemu_opt_find(opts, name); - if (opt == NULL) + 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); + } return defval; + } assert(opt->desc && opt->desc->type == QEMU_OPT_NUMBER); return opt->value.uint; } @@ -609,8 +627,13 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval) { QemuOpt *opt = qemu_opt_find(opts, name); - if (opt == NULL) + 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); + } return defval; + } assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE); return opt->value.uint; } @@ -895,17 +918,34 @@ void qemu_opts_del(QemuOpts *opts) g_free(opts); } -int qemu_opts_print(QemuOpts *opts, void *dummy) +void qemu_opts_print(QemuOpts *opts) { QemuOpt *opt; + QemuOptDesc *desc = opts->list->desc; - fprintf(stderr, "%s: %s:", opts->list->name, - opts->id ? opts->id : "<noid>"); - QTAILQ_FOREACH(opt, &opts->head, next) { - fprintf(stderr, " %s=\"%s\"", opt->name, opt->str); + if (desc[0].name == NULL) { + QTAILQ_FOREACH(opt, &opts->head, next) { + printf("%s=\"%s\" ", opt->name, opt->str); + } + return; + } + for (; desc && desc->name; desc++) { + const char *value; + QemuOpt *opt = qemu_opt_find(opts, desc->name); + + value = opt ? opt->str : desc->def_value_str; + if (!value) { + continue; + } + if (desc->type == QEMU_OPT_STRING) { + printf("%s='%s' ", desc->name, value); + } else if ((desc->type == QEMU_OPT_SIZE || + desc->type == QEMU_OPT_NUMBER) && opt) { + printf("%s=%" PRId64 " ", desc->name, opt->value.uint); + } else { + printf("%s=%s ", desc->name, value); + } } - fprintf(stderr, "\n"); - return 0; } static int opts_do_parse(QemuOpts *opts, const char *params,