Re: [Qemu-devel] [PATCH v25 06/31] QemuOpts: add qemu_opt_get_*_del functions for replace work

[Chun Yan Liu]

>>> On 4/22/2014 at 03:11 AM, in message <address@hidden>, Eric Blake
<address@hidden> wrote: 
> On 04/10/2014 11:54 AM, Chunyan Liu wrote: 
> > Add qemu_opt_get_del, qemu_opt_get_bool_del, qemu_opt_get_number_del and 
> > qemu_opt_get_size_del to replace the same handling of QEMUOptionParamter 
>  
> s/Paramter/Parameter/ 
>  
> > (get and delete). 
> >  
> > Several drivers are coded to parse a known subset of options, then 
> > remove them from the list before handing all remaining options to a 
> > second driver for further option processing.  get_*_del makes it easier 
> > to retrieve a known option (or its default) and remove it from the list 
> > all in one action. 
> >  
> > Share common helper function: 
> >  
> > For qemu_opt_get_bool/size/number, they and their get_*_del counterpart 
> > could share most of the code except whether or not deleting the opt from 
> > option list, so generate common helper functions. 
> >  
> > For qemu_opt_get and qemu_opt_get_del, keep code duplication, since 
> > 1. qemu_opt_get_del returns malloc'd memory while qemu_opt_get returns 
> > in-place memory 
> > 2. qemu_opt_get_del returns (char *), qemu_opt_get returns (const char *), 
> > and could not change to (char *), since in one case, it will return 
> > desc->def_value_str, which is (const char *). 
> >  
> > Signed-off-by: Dong Xu Wang <address@hidden> 
> > Signed-off-by: Chunyan Liu <address@hidden> 
> > --- 
> >  include/qemu/option.h |   6 +++ 
> >  util/qemu-option.c    | 116  
> ++++++++++++++++++++++++++++++++++++++++++++------ 
> >  2 files changed, 109 insertions(+), 13 deletions(-) 
> >  
>  
> > +++ b/util/qemu-option.c 
> > @@ -575,6 +575,19 @@ static void qemu_opt_del(QemuOpt *opt) 
> >      g_free(opt); 
> >  } 
> >   
> > +/* qemu_opt_set allow many settings for the same option. 
>  
> s/allow/allows/ 
>  
> Question for a possible future patch: who relies on qemu_opt_set 
> supporting multiple settings for the same option?  Would it make the 

I checked the commit log, it changed into "Never overwrite a QemuOpt"
in this commit for cases like "-net user,hostfwd=" :
dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9
Not sure if it is stilled used.

> code any cleaner to rework qemu_opt_set to track only one value for an 
> option, with the caller having the option of either flagging duplicates 
> as an error or silently replacing the old option value with the new? 
> But that doesn't belong in this patch, so it doesn't impact my review. 
>  
> > + * This function is to delete all settings for an option. 
>  
> s/is to delete/deletes/ 
>  
> > +/* Get a known option (or its default) and remove it from the list 
> > + * all in one action. Return a malloced string of the option vaule. 
>  
> s/vaule/value/ 
>  
> As my findings on this patch are limited to typo fixes, it is trivial 
> enough that you can fix them and add: 
> Reviewed-by: Eric Blake <address@hidden> 
>  
> --  
> Eric Blake   eblake redhat com    +1-919-301-3266 
> Libvirt virtualization library http://libvirt.org 
>  
>