Re: [Qemu-devel] [PATCH v20 03/26] improve some functions in qemu-option.c

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

On 02/11/2014 11:33 PM, Chunyan Liu wrote:
> Improve opt_get and opt_set group of functions. For opt_get, check and handle
> NUlL input; for opt_set, when set to an existing option, rewrite the option

s/NUlL/NULL/

> with new value.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  util/qemu-option.c |   84 +++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 70 insertions(+), 14 deletions(-)
> 
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index fd84f95..ea6793a 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -499,6 +499,9 @@ static QemuOpt *qemu_opt_find(QemuOpts *opts, const char *name)
>  {
>      QemuOpt *opt;
>  
> +    if (!opts)
> +        return NULL;
> +

Why not just 'assert(opts);'?  In other words, what caller is planning
on passing a NULL opts, since I couldn't find any existing caller that
did that?  Please update the commit message with justification of how
this can simplify a caller in a later patch, if that is your plan; but
without that justification, I'm going solely off the diffstat and the
fact that no existing caller passes NULL, and concluding that this
change is bloat.

> +    opt = qemu_opt_find(opts, name);
>      if (!opt) {

> +
> +    opt = qemu_opt_find(opts, name);
> +
>      if (opt == NULL) {

Inconsistent styles here; might be worth making them all look alike
while touching them, if we keep this patch.

> @@ -664,6 +693,13 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>          return;
>      }
>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);
> +        opt->str = g_strdup(value);

Ugg.  Why did the struct declare things as const char *str if we are
going to be strdup'ing into it?  I worry about attempting to free
non-malloc'd memory any time I see a cast to lose a const.  Is this just
a case of someone incorrectly trying to be const-correct, and now you
have to work around it?  If you drop the 'const' from option_int.h, do
things still compile?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]