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

[Eric Blake]

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.

> +    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 >=).

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

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

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

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

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

signature.asc
Description: OpenPGP digital signature