qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 02/54] char: use a const CharDriver


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 02/54] char: use a const CharDriver
Date: Tue, 13 Dec 2016 17:11:59 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1

On 12/12/2016 04:42 PM, Marc-André Lureau wrote:
> No need to allocate & copy fileds, let's use static const struct

s/fileds/fields/

> instead.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  backends/baum.c       |   8 +++-
>  backends/msmouse.c    |   7 +++-
>  backends/testdev.c    |   7 +++-
>  qemu-char.c           | 100 
> ++++++++++++++++++++++++--------------------------
>  spice-qemu-char.c     |  14 +++++--
>  ui/console.c          |   9 +++--
>  include/sysemu/char.h |  19 +++++-----
>  7 files changed, 90 insertions(+), 74 deletions(-)
> 

> +++ b/backends/baum.c
> @@ -686,8 +686,12 @@ fail_handle:
>  
>  static void register_types(void)
>  {
> -    register_char_driver("braille", CHARDEV_BACKEND_KIND_BRAILLE, NULL,
> -                         chr_baum_init);
> +    static const CharDriver driver = {
> +        .kind = CHARDEV_BACKEND_KIND_BRAILLE,
> +        .parse = NULL, .create = chr_baum_init

Why did the "braille" string disappear?  Oh, I see... [1]

No need to specify .parse, since C99 initialization guarantees
zero-assignment to any field omitted.

I kind of prefer one struct member per line when doing C99
initialization, rather than bunching two in one line.

I also prefer a trailing comma, as then adding a new (non-zero) member
initialization in a later patch is a one-line addition, rather than
modifying an existing line to add a trailing comma.


> +++ b/backends/msmouse.c
> @@ -179,8 +179,11 @@ static CharDriverState *qemu_chr_open_msmouse(const char 
> *id,
>  
>  static void register_types(void)
>  {
> -    register_char_driver("msmouse", CHARDEV_BACKEND_KIND_MSMOUSE, NULL,
> -                         qemu_chr_open_msmouse);
> +    static const CharDriver driver = {
> +        .kind = CHARDEV_BACKEND_KIND_MSMOUSE,
> +        .parse = NULL, .create = qemu_chr_open_msmouse

Looks like my comments are repeated throughout the patch.


> -void register_char_driver(const char *name, ChardevBackendKind kind,
> -                          CharDriverParse *parse, CharDriverCreate *create)
> +void register_char_driver(const CharDriver *driver)
>  {
> -    CharDriver *s;
> -
> -    s = g_malloc0(sizeof(*s));
> -    s->name = g_strdup(name);
> -    s->kind = kind;
> -    s->parse = parse;
> -    s->create = create;
> -
> -    backends = g_slist_append(backends, s);
> +    backends = g_slist_append(backends, (void *)driver);

Might be worth a comment that this is casting away const (as my first
reaction is "oh, you forgot that C allows automatic conversion of any
pointer to void*")

>  }
>  
>  CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
> @@ -4139,7 +4123,7 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>          fprintf(stderr, "Available chardev backend types:\n");
>          for (i = backends; i; i = i->next) {
>              cd = i->data;
> -            fprintf(stderr, "%s\n", cd->name);
> +            fprintf(stderr, "%s\n", ChardevBackendKind_lookup[cd->kind]);

...[1] Your series is already long, so don't feel like you have to do
this, but: if I were working on it, I might have done the elimination of
cd->name, the name parameter, and the use of ChardevBackendKind_lookup[]
in one patch (getting rid of JUST the "braille" parameter and friends at
the call sites), and then the const'ification of the remaining
parameters in the second patch.

> +++ b/include/sysemu/char.h

Laszlo's suggestion of the git order file would have promoted this part
of the patch first :)

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