----- 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 <marcandre.lur...@redhat.com>
> > ---
> > 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
