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 <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.
> +++ 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
signature.asc
Description: OpenPGP digital signature
