[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v25 02/31] QemuOpts: add def_value_str to QemuOp
From: |
Chun Yan Liu |
Subject: |
Re: [Qemu-devel] [PATCH v25 02/31] QemuOpts: add def_value_str to QemuOptDesc |
Date: |
Wed, 23 Apr 2014 01:44:59 -0600 |
>>> On 4/22/2014 at 02:22 AM, in message <address@hidden>, Eric Blake
<address@hidden> 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 <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
>
> ...I'm fine with keeping my review.
>
> > Signed-off-by: Dong Xu Wang <address@hidden>
> > Signed-off-by: Chunyan Liu <address@hidden>
> > ---
> > 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
>
>
- [Qemu-devel] [PATCH v25 00/31] replace QEMUOptionParameter with QemuOpts, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 02/31] QemuOpts: add def_value_str to QemuOptDesc, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 05/31] QemuOpts: move qemu_opt_del ahead for later calling, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 04/31] QemuOpts: change opt->name|str from (const char *) to (char *), Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 01/31] QemuOpts: move find_desc_by_name ahead for later calling, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 03/31] qapi: output def_value_str when query command line options, Chunyan Liu, 2014/04/11
- [Qemu-devel] [PATCH v25 06/31] QemuOpts: add qemu_opt_get_*_del functions for replace work, Chunyan Liu, 2014/04/11