Re: [Qemu-devel] [PATCH v23 04/32] change opt->name and opt->str from (const char *) to (char *)

[Eric Blake <eblake@redhat.com>]

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

On 03/21/2014 04:12 AM, Chunyan Liu wrote:

Your subject says "what", but your commit message lacks a "why".
Without a good reason, it's hard to see what this patch is good for.
May I suggest:

qemu_opt_del() already assumes that all QemuOpt instances contain
malloc'd name and value; but it had to cast away const because
opts_start_struct() was doing its own thing and using static storage
instead.  By using the correct type and malloced strings everywhere, the
usage of this struct becomes clearer.

> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
>  include/qemu/option_int.h |  4 ++--
>  qapi/opts-visitor.c       | 10 +++++++---
>  util/qemu-option.c        |  4 ++--
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/include/qemu/option_int.h b/include/qemu/option_int.h
> index 8212fa4..db9ed91 100644
> --- a/include/qemu/option_int.h
> +++ 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;

While touching this, is it worth trimming the extra spacing before '*'?
 It's not like you are preserving alignment or anything.  But as that's
cosmetic, and as the code itself is correct, I'm fine if you add this
when you expand your commit message:

Reviewed-by: Eric Blake <eblake@redhat.com>

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