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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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).

Hmm good catch, I didn't realize that, the next patch fixes most of it, right?

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

Hmm, bad patch split. 

> 
> >  #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?

No

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

I think your option 2 is better

> =====
> 
> 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:

ok
> =====
> 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>
> 

Thanks, I took option 2, and fixed the remaining issues mentionned in patch 6.

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