Re: [Qemu-devel] [PATCH 05/20] char: use a const CharDriver

[Eric Blake]

On 01/05/2017 10:53 AM, Marc-André Lureau wrote:
> No need to allocate & copy fields, let's use static const struct
> instead.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/sysemu/char.h |  19 +++----
>  backends/baum.c       |   8 ++-
>  backends/msmouse.c    |   7 ++-
>  backends/testdev.c    |   7 ++-
>  qemu-char.c           | 141 
> +++++++++++++++++++++++++++++++-------------------
>  spice-qemu-char.c     |  16 ++++--
>  ui/console.c          |  10 ++--
>  7 files changed, 134 insertions(+), 74 deletions(-)
> 
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index b6e361860a..c05a896578 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -475,15 +475,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);

The old code takes a name...

> +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);

...the new code does not. Let's see why:

> +++ b/qemu-char.c
> @@ -4094,27 +4094,12 @@ static void qemu_chr_parse_udp(QemuOpts *opts, 
> ChardevBackend *backend,
>      }
>  }
>  
> -typedef struct CharDriver {
> -    const char *name;
> -    ChardevBackendKind kind;
> -    CharDriverParse *parse;
> -    CharDriverCreate *create;
> -} CharDriver;

In fact, you are moving the struct from private to public, AND dropping
a member at the same time.

> -
>  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);

The old code copies the name, the new code has no name to copy.

> -    s->kind = kind;
> -    s->parse = parse;
> -    s->create = create;
> -
> -    backends = g_slist_append(backends, s);
> +    /* casting away const */
> +    backends = g_slist_append(backends, (void *)driver);
>  }
>  
>  CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
> @@ -4139,7 +4124,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]);

The old code reused the name copied during registration, the new code
uses kind to look up the name from the QAPI enum type.  That means the
output changes: it now outputs the canonical name instead of the alias
name.  I can live with that (even if the output is temporarily weird for
listing a canonical name more than once).

> @@ -4152,7 +4137,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) {

But this change means any driver that does NOT have a name in the QAPI
enum type CANNOT be constructed from the command line, at least for the
duration of this patch.  Let's see what is impacted by this:

>  #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);

The old code registered two different names for a serial driver;

> +        {
> +            .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,
> +        },

the new code just registers the SAME driver contents twice, but both
tied to the QAPI name "serial".  Either we want _this_ patch to keep the
"tty" name around (but that's churn, because it does go away later in
the series when we add aliasing), or we can drop the second registration
as redundant, and just document that we temporarily lose access to the
aliased name until fixed in a later patch.

>  #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,
> +        },

And again.

> +        {
> +            .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,

What's weird here is that CHARDEV_BACKEND_KIND_MEMORY and
CHARDEV_BACKEND_KIND_RINGBUF are also using the same callbacks, but this
time appear as separate names in the QAPI enum.  A different approach
would be to modify qapi-schema.json to add 'tty':'ChardevHostdev' and
'parport':'ChardevHostdev' aliases, the way it already has a
'memory':ChardevRingbuf' alias, so that the above registrations that I
complained were duplicates could use the right enum values. But is it
really worth dirtying the QAPI with an enum value we don't want?
Conversely, should we remove 'memory' from QAPI as no longer supported
(yes, it breaks back-compat, but we've documented that new code should
not be using it)?

I still think you need more in the commit message to justify this code.
Option 1, minimizing code churn, documenting the temporary regression:

=====
No need to allocate & copy fields, let's use static const struct
instead.

Note that this promotes CharDriver to a public struct, but eliminates
the 'name' parameter in the process; this is because in most cases, we
can use the QAPI enum for ChardevBackend to come up with the same names,
with the only exceptions being older aliases 'tty' (for 'serial') and
'parport' (for 'parallel') where outputting the canonical name is better
anyways.  This causes a temporary regression in the ability to create a
char driver from the command line using the older spellings, but that
will be fixed in a later patch that cleans up how aliases are handled.
=====

along with this patch squashed in:
=====
diff --git i/qemu-char.c w/qemu-char.c
index 14ab287..1a39331 100644
--- i/qemu-char.c
+++ w/qemu-char.c
@@ -4930,11 +4930,7 @@ static void register_types(void)
             .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,
-        },
+        /* FIXME: Need a "tty" alias */
 #endif
 #ifdef HAVE_CHARDEV_PARPORT
         {
@@ -4942,11 +4938,7 @@ static void register_types(void)
             .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,
-        },
+        /* FIXME: Need a "parport" alias */
 #endif
 #ifdef HAVE_CHARDEV_PTY
         {
=====


Option 2, hoisting part of 6/20 earlier in the series to avoid the
temporary regression altogether:

=====
No need to allocate & copy fields, let's use static const struct
instead.

Note that this promotes CharDriver to a public struct, and replaces a
'name' parameter with an 'alias' field to handle the fact that we were
previously registering the 'serial' and 'parallel' drivers twice to pick
up their older 'tty' and 'parport' aliases (we still register the
'memory' driver as an alias for 'ringbuf', but that's because that alias
is public in the ChardevBackend QAPI type).
=====

along with this patch squashed in:
=====
diff --git i/include/sysemu/char.h w/include/sysemu/char.h
index c05a896..fee2c34 100644
--- i/include/sysemu/char.h
+++ w/include/sysemu/char.h
@@ -477,6 +477,7 @@ QemuOpts *qemu_chr_parse_compat(const char *label,
const char *filename);

 typedef struct CharDriver {
     ChardevBackendKind kind;
+    const char *alias;
     void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
     CharDriverState *(*create)(const char *id,
                                ChardevBackend *backend,
diff --git i/qemu-char.c w/qemu-char.c
index 14ab287..37219b2 100644
--- i/qemu-char.c
+++ w/qemu-char.c
@@ -4111,22 +4111,27 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts
*opts,
     GSList *i;
     ChardevReturn *ret = NULL;
     ChardevBackend *backend;
+    const char *name;
     const char *id = qemu_opts_id(opts);
     char *bid = NULL;

-    if (qemu_opt_get(opts, "backend") == NULL) {
+    name = qemu_opt_get(opts, "backend");
+    if (name == NULL) {
         error_setg(errp, "chardev: \"%s\" missing backend",
                    qemu_opts_id(opts));
         goto err;
     }

-    if (is_help_option(qemu_opt_get(opts, "backend"))) {
+    if (is_help_option(name)) {
         fprintf(stderr, "Available chardev backend types:\n");
         for (i = backends; i; i = i->next) {
             cd = i->data;
             fprintf(stderr, "%s\n", ChardevBackendKind_lookup[cd->kind]);
+            if (cd->alias) {
+                fprintf(stderr, "%s\n", cd->alias);
+            }
         }
-        exit(!is_help_option(qemu_opt_get(opts, "backend")));
+        exit(0);
     }

     if (id == NULL) {
@@ -4137,14 +4142,13 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts
*opts,
     for (i = backends; i; i = i->next) {
         cd = i->data;

-        if (strcmp(ChardevBackendKind_lookup[cd->kind],
-                   qemu_opt_get(opts, "backend")) == 0) {
+        if (strcmp(ChardevBackendKind_lookup[cd->kind], name) == 0 ||
+            g_strcmp0(cd->alias, name) == 0) {
             break;
         }
     }
     if (i == NULL) {
-        error_setg(errp, "chardev: backend \"%s\" not found",
-                   qemu_opt_get(opts, "backend"));
+        error_setg(errp, "chardev: backend \"%s\" not found", name);
         goto err;
     }

@@ -4927,11 +4931,7 @@ static void register_types(void)
 #if defined HAVE_CHARDEV_SERIAL
         {
             .kind = CHARDEV_BACKEND_KIND_SERIAL,
-            .parse = qemu_chr_parse_serial,
-            .create = qmp_chardev_open_serial,
-        },
-        {
-            .kind = CHARDEV_BACKEND_KIND_SERIAL,
+            .alias = "tty",
             .parse = qemu_chr_parse_serial,
             .create = qmp_chardev_open_serial,
         },
@@ -4939,11 +4939,7 @@ static void register_types(void)
 #ifdef HAVE_CHARDEV_PARPORT
         {
             .kind = CHARDEV_BACKEND_KIND_PARALLEL,
-            .parse = qemu_chr_parse_parallel,
-            .create = qmp_chardev_open_parallel,
-        },
-        {
-            .kind = CHARDEV_BACKEND_KIND_PARALLEL,
+            .alias = "parport",
             .parse = qemu_chr_parse_parallel,
             .create = qmp_chardev_open_parallel,
         },
=====


If you agree with either of those options, then you can add:

Reviewed-by: Eric Blake <address@hidden>

If anyone else has a strong opinion (such as changing qapi to add the
aliases 'tty' and 'parport', or even cleaning up qapi to remove 'memory'
in favor of 'ringbuf') on which of my two options is preferred, or even
going with a third option, then speak up now.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature