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

[Marc-André Lureau]

----- Original Message -----
> 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/
> 

ok

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

ok fixed

> 
> 
> > +++ 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*")

ok (note: this going away the next patch)

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

The series is pretty long already (second version even longer), I don't see 
much need to split this patch

> 
> > +++ b/include/sysemu/char.h
> 
> Laszlo's suggestion of the git order file would have promoted this part
> of the patch first :)
> 

done

thanks