>>> On 4/22/2014 at 03:11 AM, in message <[email protected]>, Eric >>> Blake <[email protected]> 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 <[email protected]> > > Signed-off-by: Chunyan Liu <[email protected]> > > --- > > 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 <[email protected]> > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > >
