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

[Eric Blake <eblake@redhat.com>]

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

On 03/10/2014 01:31 AM, 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
> with new value.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  include/qemu/option_int.h |  4 +--
>  util/qemu-option.c        | 81 +++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 16 deletions(-)
> 
> 
> +++ b/include/qemu/option_int.h
> @@ -30,8 +30,8 @@
>  #include "qemu/error-report.h"
>  
>  struct QemuOpt {
> -    const char   *name;
> -    const char   *str;
> +    char   *name;
> +    char   *str;

You are losing the 'const' here...


> @@ -704,6 +730,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);

...which means the cast is pointless here.

Hmm.  This means that you are giving opt_set() the behavior of 'last
version wins', by silently overwriting earlier versions.  If I'm
understanding the existing code correctly, the previous behavior was
that calling opt_set twice in a row on the same name would inject BOTH
names into 'opts', but then subsequent lookups on opts would find the
FIRST hit.  Doesn't that mean this is a semantic change:

qemu -opt key=value1,key=value2

would previously set key to value1, but now sets key to value2.  Is it
intentional?  If so, document it in the commit message.  Or am I missing
something, where we already reject duplicates, in which case, your new
code is currently unreachable, and your commit message could do with
more explanation of why you are adding something that later patches will
make use of.

Also, I'd feel a LOT better if we FIRST had a testsuite that covered
what QemuOpts is _supposed_ to do before we then patch it,so that we
aren't getting surprised by silent changes in behavior.  It's okay to
write a test that shows old behavior, then tweak the test in the patch
that updates the behavior along with the justification of why the new
behavior is nicer.

> @@ -744,16 +777,24 @@ void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)

> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);

Another pointless cast.


>  
> +    opt = qemu_opt_find(opts, name);
> +    if (opt) {
> +        g_free((char *)opt->str);

and another.

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