[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 06/20] char: use a static array for backends
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 06/20] char: use a static array for backends |
Date: |
Fri, 6 Jan 2017 18:22:34 -0500 (EST) |
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
- Re: [Qemu-devel] [PATCH 02/20] qemu-options: stdio is available on win32, (continued)
- [Qemu-devel] [PATCH 03/20] char: add qemu_chr_fe_add_watch() Returns description, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 04/20] doc: fix spelling, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 05/20] char: use a const CharDriver, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 06/20] char: use a static array for backends, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 08/20] char: fold single-user functions in caller, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 09/20] char: introduce generic qemu_chr_get_kind(), Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 10/20] char: use a feature bit for replay, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 12/20] bt: use qemu_chr_alloc(), Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 07/20] char: move callbacks in CharDriver, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 14/20] char: rename TCPChardev and NetChardev, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 11/20] char: allocate CharDriverState as a single object, Marc-André Lureau, 2017/01/05
- [Qemu-devel] [PATCH 15/20] spice-char: improve error reporting, Marc-André Lureau, 2017/01/05