Re: [Qemu-devel] [PATCH 06/20] char: use a static array for backends

[Marc-André Lureau]

Hi

----- Original Message -----
> On 01/05/2017 10:53 AM, Marc-André Lureau wrote:
> > Number and kinds of backends is known at compile-time, use a fixed-sized
> > static array to simplify iterations & lookups. Add an alias field to the
> > CharDriver structure to cover the cases where we previously registered a
> > driver twice under two names.
> 
> Except that 5/20 didn't register twice under two names, so my option 2
> on that patch was floating that portion of this patch earlier.
> 
> > 
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  include/sysemu/char.h |  1 +
> >  qemu-char.c           | 98
> >  ++++++++++++++++++++++++++++++---------------------
> >  2 files changed, 59 insertions(+), 40 deletions(-)
> > 
> > @@ -4134,17 +4139,20 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts
> > *opts,
> >          goto err;
> >      }
> >  
> > -    for (i = backends; i; i = i->next) {
> > -        cd = i->data;
> > +    cd = NULL;
> 
> Dead assignment, since backends is a non-empty array so it will always
> be overwritten by the first iteration of the following loop.

ok, it feels more correct. I'll move it at declaration instead if you don't 
mind.

> 
> > +    for (i = 0; i < ARRAY_SIZE(backends); i++) {
> > +        cd = backends[i];
> >  
> > -        if (strcmp(ChardevBackendKind_lookup[cd->kind],
> > -                   qemu_opt_get(opts, "backend")) == 0) {
> > +        if (!cd) {
> > +            continue;
> > +        }
> > +        if (g_str_equal(ChardevBackendKind_lookup[cd->kind], name) ||
> > +            (g_strcmp0(cd->alias, name) == 0)) {
> >              break;
> >          }
> >      }
> > -    if (i == NULL) {
> > -        error_setg(errp, "chardev: backend \"%s\" not found",
> > -                   qemu_opt_get(opts, "backend"));
> > +    if (cd == NULL || i >= ARRAY_SIZE(backends)) {
> 
> Simplify: 'if (i == ARRAY_SIZE(backends))' (whether cd is NULL at the
> end of the loop is irrelevant, you know it is non-NULL if you broke
> early on a match, but it is ALSO non-null if the last element of the
> array was non-NULL; and right now, our last element of the array is the
> 'memory' alias which is unconditionally present and thus non-NULL; also,
> i cannot grow greater than ARRAY_SIZE(), so == suffices rather than >=).

ok

> 
> >  ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
> >  {
> >      ChardevBackendInfoList *backend_list = NULL;
> > -    CharDriver *c = NULL;
> > -    GSList *i = NULL;
> > +    const CharDriver *c;
> > +    int i;
> >  
> > -    for (i = backends; i; i = i->next) {
> > -        ChardevBackendInfoList *info = g_malloc0(sizeof(*info));
> > -        c = i->data;
> > -        info->value = g_malloc0(sizeof(*info->value));
> > -        info->value->name = g_strdup(ChardevBackendKind_lookup[c->kind]);
> > +    for (i = 0; i < ARRAY_SIZE(backends); i++) {
> > +        c = backends[i];
> > +        if (!c) {
> > +            continue;
> > +        }
> >  
> > -        info->next = backend_list;
> > -        backend_list = info;
> > +        backend_list = qmp_prepend_backend(backend_list, c,
> > +
> > ChardevBackendKind_lookup[c->kind]);
> > +        if (c->alias) {
> > +            backend_list = qmp_prepend_backend(backend_list, c, c->alias);
> > +        }
> 
> Hmm, I didn't account for this in the changes I suggested for option 2
> of 5/20 (which means unless you further tweak it, I temporarily
> regressed the command to omit the aliases - but then again, taking your
> 5/20 as-is without either of my options omits the aliases and lists the
> canonical name twice).

fixed in patch 5

> 
> > @@ -4827,22 +4849,16 @@ ChardevReturn *qmp_chardev_add(const char *id,
> > ChardevBackend *backend,
> >          goto out_error;
> >      }
> >  
> > -    for (i = backends; i; i = i->next) {
> > -        cd = i->data;
> > -
> > -        if (cd->kind == backend->type) {
> > -            chr = cd->create(id, backend, ret, &be_opened, &local_err);
> > -            if (local_err) {
> > -                error_propagate(errp, local_err);
> > -                goto out_error;
> > -            }
> > -            break;
> > -        }
> > +    cd = (int)backend->type >= 0 && backend->type < ARRAY_SIZE(backends) ?
> > +        backends[backend->type] : NULL;
> > +    if (cd == NULL) {
> > +        error_setg(errp, "chardev backend not available");
> 
> Worth including %s and the user-provided name that failed lookup?

It's improved with qom-ify patch

> 
> > @@ -4927,6 +4943,7 @@ static void register_types(void)
> >  #if defined HAVE_CHARDEV_SERIAL
> >          {
> >              .kind = CHARDEV_BACKEND_KIND_SERIAL,
> > +            .alias = "tty",
> >              .parse = qemu_chr_parse_serial,
> >              .create = qmp_chardev_open_serial,
> >          },
> 
> Unless you get rid of the duplicates (both my option 1 and option 2 of
> 5/20 did so), then you are registering 'serial/tty' once, and
> 'serial/NULL' a second time, where the second registration overwrites
> the first, and so the alias doesn't work.

bad patch split :)

> 
> > @@ -4939,6 +4956,7 @@ static void register_types(void)
> >  #ifdef HAVE_CHARDEV_PARPORT
> >          {
> >              .kind = CHARDEV_BACKEND_KIND_PARALLEL,
> > +            .alias = "parport",
> >              .parse = qemu_chr_parse_parallel,
> >              .create = qmp_chardev_open_parallel,
> >          },
> 
> And again.

thanks