Re: [Qemu-devel] [PATCH v25 02/31] QemuOpts: add def_value_str to QemuOptDesc

[Eric Blake]

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 <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

signature.asc
Description: OpenPGP digital signature