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

["Marc-André Lureau" <mlureau@redhat.com>]

Hi

----- Original Message -----
> 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 <marcandre.lureau@redhat.com>
> > ---
> >  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.

ok

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

No it doesnt, but g_strcmp0 does, let's switch to it.

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

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

I don't think it matters. Furthermore, this code is temporary, the "char: get rid of CharDriver" patch will use the same function qmp_query_chardev_backends() to build the list with object_class_get_list() order.

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

No, it shouldn't break. There were 2 CharDrivers in case of alias chardev. It uses the "kind" field only to lookup the corresponding name.

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

Thus I don't see much point in changing the order.

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

neither

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