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

[Chun Yan Liu]

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