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