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

[Eric Blake]

On 12/12/2016 04:42 PM, Marc-André Lureau wrote:
> Number and kinds of backends is known at compile-time, use a fixed-sized
> static array to simplify iterations & lookups.
> 
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  qemu-char.c           | 101 
> ++++++++++++++++++++++++++++----------------------
>  include/sysemu/char.h |   1 +
>  2 files changed, 58 insertions(+), 44 deletions(-)
> 

> @@ -4121,9 +4121,14 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
>  
>      if (is_help_option(qemu_opt_get(opts, "backend"))) {
>          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]);
> +        for (i = 0; i < ARRAY_SIZE(backends); i++) {
> +            cd = backends[i];
> +            if (cd) {
> +                fprintf(stderr, "%s\n", ChardevBackendKind_lookup[cd->kind]);
> +                if (cd->alias) {
> +                    fprintf(stderr, "%s\n", cd->alias);
> +                }
> +            }

Where did cd->alias come from?
/me reads rest of patch
Oh, you added it in the .h file. All the more reason to use patch
ordering to list .h changes first, but may also be worth a mention in
the commit message that you add an alias field to cover the cases where
we previously registered a driver twice under two names.

> 
> -        cd = i->data;
> +    cd = NULL;
> +    for (i = 0; i < ARRAY_SIZE(backends); i++) {
> +        const char *name = qemu_opt_get(opts, "backend");
> +        cd = backends[i];

Please hoist the computation of 'name' outside of the loop...

> -        if (strcmp(ChardevBackendKind_lookup[cd->kind],
> -                   qemu_opt_get(opts, "backend")) == 0) {
> +        if (!cd) {
> +            continue;
> +        }
> +        if (g_str_equal(ChardevBackendKind_lookup[cd->kind], name) ||
> +            (cd->alias && g_str_equal(cd->alias, name))) {

Doesn't g_str_equal(NULL, "str") do the right thing without crashing?
Therefore, no need to check if cd->alias is non-NULL.

>              break;
>          }
>      }
> -    if (i == NULL) {
> +    if (cd == NULL) {
>          error_setg(errp, "chardev: backend \"%s\" not found",
>                     qemu_opt_get(opts, "backend"));

...and then reuse 'name' here as well, rather than making multiple
repeated calls to qemu_opt_get().

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

The help text now lists aliases immediately after the canonical name,
while the QMP command now prepends aliases prior to the canonical name.
It doesn't affect correctness, but should you try to make the two lists
appear in the same order?  (Presumably by prepending the alias first,
not second.)


> @@ -4907,16 +4925,11 @@ static void register_types(void)
>          { .kind = CHARDEV_BACKEND_KIND_STDIO,
>            .parse = qemu_chr_parse_stdio, .create = qemu_chr_open_stdio },
>  #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,
> +        { .kind = CHARDEV_BACKEND_KIND_SERIAL, .alias = "tty",
>            .parse = qemu_chr_parse_serial, .create = qmp_chardev_open_serial 
> },

Wait. How did patch 2/54 work? Or did you temporarily break the 'tty'
alias in that patch, and then fix it here?  All the more reason that my
review of 2/54 complaining about getting rid of the cd->name field
should be a separate patch; now I'm convinced of it.  But in that patch,
I suggested the order:

remove cd->name field as redundant with enum array lookup
convert to struct without cd->name

Whereas with this patch in the mix, the steps should probably be:

convert to struct that still has cd->name
add cd->alias to have multiple names
remove cd->name field as redundant with enum array lookup

>  #endif
>  #ifdef HAVE_CHARDEV_PARPORT
> -        { .kind = CHARDEV_BACKEND_KIND_PARALLEL,
> -          .parse = qemu_chr_parse_parallel,
> -          .create = qmp_chardev_open_parallel },
> -        { .kind = CHARDEV_BACKEND_KIND_PARALLEL,
> +        { .kind = CHARDEV_BACKEND_KIND_PARALLEL, .alias = "parport",
>            .parse = qemu_chr_parse_parallel,
>            .create = qmp_chardev_open_parallel },

Again, another breakage in 2/54.

>  #endif
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 144514c962..c8750ede21 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -474,6 +474,7 @@ void qemu_chr_set_feature(CharDriverState *chr,
>  QemuOpts *qemu_chr_parse_compat(const char *label, const char *filename);
>  
>  typedef struct CharDriver {
> +    const char *alias;
>      ChardevBackendKind kind;
>      void (*parse)(QemuOpts *opts, ChardevBackend *backend, Error **errp);
>      CharDriverState *(*create)(const char *id,
> 

Overall, I like the direction this is headed in.

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

signature.asc
Description: OpenPGP digital signature