>>> On 4/22/2014 at 02:22 AM, in message <535561e2.3000...@redhat.com>, Eric >>> Blake <ebl...@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 <ebl...@redhat.com> > > ...I'm fine with keeping my review. > > > Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com> > > Signed-off-by: Chunyan Liu <cy...@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 > >
