diff mbox

[02/54] char: use a const CharDriver

Message ID 20161212224325.20790-3-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Dec. 12, 2016, 10:42 p.m. UTC
No need to allocate & copy fileds, let's use static const struct
instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@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(-)

Comments

Eric Blake Dec. 13, 2016, 11:11 p.m. UTC | #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 <marcandre.lureau@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 :)
Marc-Andre Lureau Jan. 2, 2017, 3:33 p.m. UTC | #2
----- 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.lureau@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
diff mbox

Patch

diff --git a/backends/baum.c b/backends/baum.c
index b92369d840..a790622867 100644
--- a/backends/baum.c
+++ 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
+    };
+
+    register_char_driver(&driver);
 }
 
 type_init(register_types);
diff --git a/backends/msmouse.c b/backends/msmouse.c
index 733ca80f48..567000860c 100644
--- a/backends/msmouse.c
+++ 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
+    };
+    register_char_driver(&driver);
 }
 
 type_init(register_types);
diff --git a/backends/testdev.c b/backends/testdev.c
index 60156e320e..9bd86238d2 100644
--- a/backends/testdev.c
+++ b/backends/testdev.c
@@ -130,8 +130,11 @@  static CharDriverState *chr_testdev_init(const char *id,
 
 static void register_types(void)
 {
-    register_char_driver("testdev", CHARDEV_BACKEND_KIND_TESTDEV, NULL,
-                         chr_testdev_init);
+    static const CharDriver driver = {
+        .kind = CHARDEV_BACKEND_KIND_TESTDEV,
+        .parse = NULL, .create = chr_testdev_init
+    };
+    register_char_driver(&driver);
 }
 
 type_init(register_types);
diff --git a/qemu-char.c b/qemu-char.c
index 2c9940cea4..309709cb6d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4094,27 +4094,11 @@  static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
     }
 }
 
-typedef struct CharDriver {
-    const char *name;
-    ChardevBackendKind kind;
-    CharDriverParse *parse;
-    CharDriverCreate *create;
-} CharDriver;
-
 static GSList *backends;
 
-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);
 }
 
 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]);
         }
         exit(!is_help_option(qemu_opt_get(opts, "backend")));
     }
@@ -4152,7 +4136,8 @@  CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
     for (i = backends; i; i = i->next) {
         cd = i->data;
 
-        if (strcmp(cd->name, qemu_opt_get(opts, "backend")) == 0) {
+        if (strcmp(ChardevBackendKind_lookup[cd->kind],
+                   qemu_opt_get(opts, "backend")) == 0) {
             break;
         }
     }
@@ -4371,7 +4356,7 @@  ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
         ChardevBackendInfoList *info = g_malloc0(sizeof(*info));
         c = i->data;
         info->value = g_malloc0(sizeof(*info->value));
-        info->value->name = g_strdup(c->name);
+        info->value->name = g_strdup(ChardevBackendKind_lookup[c->kind]);
 
         info->next = backend_list;
         backend_list = info;
@@ -4907,45 +4892,56 @@  void qemu_chr_cleanup(void)
 
 static void register_types(void)
 {
-    register_char_driver("null", CHARDEV_BACKEND_KIND_NULL, NULL,
-                         qemu_chr_open_null);
-    register_char_driver("socket", CHARDEV_BACKEND_KIND_SOCKET,
-                         qemu_chr_parse_socket, qmp_chardev_open_socket);
-    register_char_driver("udp", CHARDEV_BACKEND_KIND_UDP, qemu_chr_parse_udp,
-                         qmp_chardev_open_udp);
-    register_char_driver("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
-                         qemu_chr_parse_ringbuf, qemu_chr_open_ringbuf);
-    register_char_driver("file", CHARDEV_BACKEND_KIND_FILE,
-                         qemu_chr_parse_file_out, qmp_chardev_open_file);
-    register_char_driver("stdio", CHARDEV_BACKEND_KIND_STDIO,
-                         qemu_chr_parse_stdio, qemu_chr_open_stdio);
+    int i;
+    static const CharDriver drivers[] = {
+        { .kind = CHARDEV_BACKEND_KIND_NULL, .parse = NULL,
+          .create = qemu_chr_open_null },
+        { .kind = CHARDEV_BACKEND_KIND_SOCKET,
+          .parse = qemu_chr_parse_socket, .create = qmp_chardev_open_socket },
+        { .kind = CHARDEV_BACKEND_KIND_UDP, .parse = qemu_chr_parse_udp,
+          .create = qmp_chardev_open_udp },
+        { .kind = CHARDEV_BACKEND_KIND_RINGBUF,
+          .parse = qemu_chr_parse_ringbuf, .create = qemu_chr_open_ringbuf },
+        { .kind = CHARDEV_BACKEND_KIND_FILE,
+          .parse = qemu_chr_parse_file_out, .create = qmp_chardev_open_file },
+        { .kind = CHARDEV_BACKEND_KIND_STDIO,
+          .parse = qemu_chr_parse_stdio, .create = qemu_chr_open_stdio },
 #if defined HAVE_CHARDEV_SERIAL
-    register_char_driver("serial", CHARDEV_BACKEND_KIND_SERIAL,
-                         qemu_chr_parse_serial, qmp_chardev_open_serial);
-    register_char_driver("tty", CHARDEV_BACKEND_KIND_SERIAL,
-                         qemu_chr_parse_serial, qmp_chardev_open_serial);
+        { .kind = CHARDEV_BACKEND_KIND_SERIAL,
+          .parse = qemu_chr_parse_serial, .create = qmp_chardev_open_serial },
+        { .kind = CHARDEV_BACKEND_KIND_SERIAL,
+          .parse = qemu_chr_parse_serial, .create = qmp_chardev_open_serial },
 #endif
 #ifdef HAVE_CHARDEV_PARPORT
-    register_char_driver("parallel", CHARDEV_BACKEND_KIND_PARALLEL,
-                         qemu_chr_parse_parallel, qmp_chardev_open_parallel);
-    register_char_driver("parport", CHARDEV_BACKEND_KIND_PARALLEL,
-                         qemu_chr_parse_parallel, qmp_chardev_open_parallel);
+        { .kind = CHARDEV_BACKEND_KIND_PARALLEL,
+          .parse = qemu_chr_parse_parallel,
+          .create = qmp_chardev_open_parallel },
+        { .kind = CHARDEV_BACKEND_KIND_PARALLEL,
+          .parse = qemu_chr_parse_parallel,
+          .create = qmp_chardev_open_parallel },
 #endif
 #ifdef HAVE_CHARDEV_PTY
-    register_char_driver("pty", CHARDEV_BACKEND_KIND_PTY, NULL,
-                         qemu_chr_open_pty);
+        { .kind = CHARDEV_BACKEND_KIND_PTY,
+          .parse = NULL, .create = qemu_chr_open_pty },
 #endif
 #ifdef _WIN32
-    register_char_driver("console", CHARDEV_BACKEND_KIND_CONSOLE, NULL,
-                         qemu_chr_open_win_con);
+        { .kind = CHARDEV_BACKEND_KIND_CONSOLE, .parse = NULL,
+          .create = qemu_chr_open_win_con },
 #endif
-    register_char_driver("pipe", CHARDEV_BACKEND_KIND_PIPE,
-                         qemu_chr_parse_pipe, qemu_chr_open_pipe);
-    register_char_driver("mux", CHARDEV_BACKEND_KIND_MUX, qemu_chr_parse_mux,
-                         qemu_chr_open_mux);
-    /* Bug-compatibility: */
-    register_char_driver("memory", CHARDEV_BACKEND_KIND_MEMORY,
-                         qemu_chr_parse_ringbuf, qemu_chr_open_ringbuf);
+        { .kind = CHARDEV_BACKEND_KIND_PIPE,
+          .parse = qemu_chr_parse_pipe, .create = qemu_chr_open_pipe },
+        { .kind = CHARDEV_BACKEND_KIND_MUX, .parse = qemu_chr_parse_mux,
+          .create = qemu_chr_open_mux },
+        /* Bug-compatibility: */
+        { .kind = CHARDEV_BACKEND_KIND_MEMORY,
+          .parse = qemu_chr_parse_ringbuf, .create = qemu_chr_open_ringbuf },
+    };
+
+
+    for (i = 0; i < ARRAY_SIZE(drivers); i++) {
+        register_char_driver(&drivers[i]);
+    }
+
     /* this must be done after machine init, since we register FEs with muxes
      * as part of realize functions like serial_isa_realizefn when -nographic
      * is specified
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 276c4aef68..8e9151db5c 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -389,10 +389,16 @@  static void qemu_chr_parse_spice_port(QemuOpts *opts, ChardevBackend *backend,
 
 static void register_types(void)
 {
-    register_char_driver("spicevmc", CHARDEV_BACKEND_KIND_SPICEVMC,
-                         qemu_chr_parse_spice_vmc, qemu_chr_open_spice_vmc);
-    register_char_driver("spiceport", CHARDEV_BACKEND_KIND_SPICEPORT,
-                         qemu_chr_parse_spice_port, qemu_chr_open_spice_port);
+    static const CharDriver vmc_driver = {
+        .kind = CHARDEV_BACKEND_KIND_SPICEVMC,
+        .parse = qemu_chr_parse_spice_vmc, .create = qemu_chr_open_spice_vmc
+    };
+    static const CharDriver port_driver = {
+        .kind = CHARDEV_BACKEND_KIND_SPICEPORT,
+        .parse = qemu_chr_parse_spice_port, .create = qemu_chr_open_spice_port
+    };
+    register_char_driver(&vmc_driver);
+    register_char_driver(&port_driver);
 }
 
 type_init(register_types);
diff --git a/ui/console.c b/ui/console.c
index ed888e55ea..764501b363 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -2180,12 +2180,15 @@  static const TypeInfo qemu_console_info = {
     .class_size = sizeof(QemuConsoleClass),
 };
 
-
 static void register_types(void)
 {
+    static const CharDriver vc_driver = {
+        .kind = CHARDEV_BACKEND_KIND_VC,
+        .parse = qemu_chr_parse_vc, .create = vc_init
+    };
+
     type_register_static(&qemu_console_info);
-    register_char_driver("vc", CHARDEV_BACKEND_KIND_VC, qemu_chr_parse_vc,
-                         vc_init);
+    register_char_driver(&vc_driver);
 }
 
 type_init(register_types);
diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 0a149428cf..144514c962 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -473,15 +473,16 @@  void qemu_chr_set_feature(CharDriverState *chr,
                           CharDriverFeature feature);
 QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
 
-typedef void CharDriverParse(QemuOpts *opts, ChardevBackend *backend,
-                             Error **errp);
-typedef CharDriverState *CharDriverCreate(const char *id,
-                                          ChardevBackend *backend,
-                                          ChardevReturn *ret, bool *be_opened,
-                                          Error **errp);
-
-void register_char_driver(const char *name, ChardevBackendKind kind,
-                          CharDriverParse *parse, CharDriverCreate *create);
+typedef struct CharDriver {
+    ChardevBackendKind kind;
+    void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
+    CharDriverState *(*create)(const char *id,
+                               ChardevBackend *backend,
+                               ChardevReturn *ret, bool *be_opened,
+                               Error **errp);
+} CharDriver;
+
+void register_char_driver(const CharDriver *driver);
 
 extern int term_escape_char;