qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v25 06/31] QemuOpts: add qemu_opt_get_*_del func


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH v25 06/31] QemuOpts: add qemu_opt_get_*_del functions for replace work
Date: Mon, 21 Apr 2014 13:11:08 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]