Re: [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver

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

Hi

----- Original Message -----
> On 01/11/2017 11:29 AM, Marc-André Lureau wrote:
> > qemu_chr_new_from_opts() is modified to not need CharDriver backend[]
> > array, but uses instead objectified qmp_query_chardev_backends() and
> > char_get_class(). The alias field is moved outside in a ChardevAlias[],
> > similar to QDevAlias for devices.
> > 
> > "kind" and "parse" are moved to ChardevClass ("kind" is to be removed
> > next)
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> 
> 
> >  Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
> >                                  Error **errp)
> >  {
> >      Error *local_err = NULL;
> > -    const CharDriver *cd = NULL;
> > +    const ChardevClass *cc;
> >      Chardev *chr;
> >      int i;
> >      ChardevReturn *ret = NULL;
> > @@ -4180,16 +4196,13 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
> >  
> >      if (is_help_option(name)) {
> >          GString *str = g_string_new("");
> > -        for (i = 0; i < ARRAY_SIZE(backends); i++) {
> > -            cd = backends[i];
> > -            if (cd) {
> > -                g_string_append_printf(str, "\n%s",
> > ChardevBackendKind_lookup[cd->kind]);
> > -                if (cd->alias) {
> > -                    g_string_append_printf(str, "\n%s", cd->alias);
> > -                }
> > -            }
> > +        ChardevBackendInfoList *l, *il = qmp_query_chardev_backends(NULL);
> > +
> > +        for (l = il; l != NULL; l = l->next) {
> > +            g_string_append_printf(str, "\n%s", l->value->name);
> >          }
> >  
> > +        qapi_free_ChardevBackendInfoList(l);
> 
> Is this really the most efficient way to do it?  I guess it works, but
> consider what qmp_query_chardev_backends() is doing: it is iterating
> through a list of chardevs, and for each one (or its alias), calling a
> callback qmp_prepend_backend().  Would a better factorization be to have
> a common function that takes care of the iteration and takes a function
> pointer for what to do on each element of the iteration, and then
> qmp_query_chardev_backend calls helper(qmp_prepend_backup) while this
> code calls helper(append_name), so that you aren't wasting the creation
> of an entire ChardevBackendInfoList item just for the action of
> appending the name?
> 

I don't think speed matters here, but ok

> > @@ -4407,21 +4417,23 @@ qmp_prepend_backend(ChardevBackendInfoList *list,
> > const char *name)
> >  ChardevBackendInfoList *qmp_query_chardev_backends(Error **errp)
> >  {
> >      ChardevBackendInfoList *backend_list = NULL;
> > -    const CharDriver *c;
> > +    GSList *l, *list = object_class_get_list(TYPE_CHARDEV, false);
> >      int i;
> >  
> > -    for (i = 0; i < ARRAY_SIZE(backends); i++) {
> > -        c = backends[i];
> > -        if (!c) {
> > +    for (l = list; l; l = l->next) {
> > +        ObjectClass *klass = l->data;
> > +        assert(g_str_has_prefix(object_class_get_name(klass),
> > "chardev-"));
> > +        if (CHARDEV_CLASS(klass)->internal) {
> >              continue;
> >          }
> > -
> >          backend_list = qmp_prepend_backend(backend_list,
> > -
> > ChardevBackendKind_lookup[c->kind]);
> > -        if (c->alias) {
> > -            backend_list = qmp_prepend_backend(backend_list, c->alias);
> > -        }
> > +                                           object_class_get_name(klass) +
> > 8);
> > +    }
> > +    for (i = 0; i < ARRAY_SIZE(chardev_alias_table); i++) {
> > +        backend_list = qmp_prepend_backend(backend_list,
> > +                                           chardev_alias_table[i].alias);
> 
> This changes the order of the list which is output to the user
> (shouldn't matter in practice, but may be worth mentioning in the commit
> message as intentionally safe).

ok

> 
> Again, I think that a little more refactoring to have the iteration over
> object_class_get_list() and chardev_alias_table[], coupled with a
> callback passed in by the client (with clients qemu_chr_new_from_opts
> and qmp_query_chardev_backends), will separate the iteration from the
> action, so that you don't always have to create a ChardevBackendInfoList
> that will just be thrown away.

not a trivial refactoring (various callbacks, passed to callback etc) but ok

> 
> >      }
> > +    g_slist_free(l);
> 
> Shouldn't this be g_slist_free(list)?

Yes my bad, same error with qapi_free_ChardevBackendInfoList()!

> 
> > @@ -4983,35 +4978,6 @@ void qemu_chr_set_feature(Chardev *chr,
> >      return set_bit(feature, chr->features);
> >  }
> >  
> > -static const ChardevClass *char_get_class(const char *driver, Error
> > **errp)
> > -{
> 
> This function is just moved earlier, which gets a little messy with the
> rest of the patch. Can you split the code motion into a separate patch
> for ease of review?  In fact, since it was just barely added in "[PATCH
> v2 20/20] chardev: qom-ify]" and that patch hasn't landed yet, can't you
> just amend that patch to hoist it early enough in the first place,
> rather than churning it?

It depends if I get to resend it or not. Paolo, you may drop it from the first series, so I resend it with this one?

> 
> 
> > @@ -5107,45 +5073,34 @@ void qemu_chr_cleanup(void)
> >  
> >  static void register_types(void)
> >  {
> > -    static const struct {
> > -        const CharDriver *driver;
> > -        const TypeInfo *type;
> > -    } chardevs[] = {
> > -        { &null_driver, &char_null_type_info },
> > -        { &socket_driver, &char_socket_type_info },
> > -        { &udp_driver, &char_udp_type_info },
> > -        { &ringbuf_driver, &char_ringbuf_type_info },
> > -        { &file_driver, &char_file_type_info },
> > -        { &stdio_driver, &char_stdio_type_info },
> > +    type_register_static(&char_type_info);
> > +#ifndef _WIN32
> > +    type_register_static(&char_fd_type_info);
> > +#else
> > +    type_register_static(&char_win_type_info);
> > +    type_register_static(&char_win_stdio_type_info);
> > +#endif
> 
> It's a bit weird that these are registered first,...
> 
> > +    type_register_static(&char_null_type_info);
> > +    type_register_static(&char_socket_type_info);
> > +    type_register_static(&char_udp_type_info);
> > +    type_register_static(&char_ringbuf_type_info);
> > +    type_register_static(&char_file_type_info);
> > +    type_register_static(&char_stdio_type_info);
> >  #ifdef HAVE_CHARDEV_SERIAL
> > -        { &serial_driver, &char_serial_type_info },
> > +    type_register_static(&char_serial_type_info);
> >  #endif
> >  #ifdef HAVE_CHARDEV_PARPORT
> > -        { &parallel_driver, &char_parallel_type_info },
> > +    type_register_static(&char_parallel_type_info);
> >  #endif
> >  #ifdef HAVE_CHARDEV_PTY
> > -        { &pty_driver, &char_pty_type_info },
> > +    type_register_static(&char_pty_type_info);
> >  #endif
> >  #ifdef _WIN32
> > -        { &console_driver, &char_console_type_info },
> > +    type_register_static(&char_console_type_info);
> 
> Any reason we can't consolidate the _WIN32 ifdefs into one place?

we could, but this is going away later in this series

> 
> >  #endif
> > -        { &pipe_driver, &char_pipe_type_info },
> > -        { &mux_driver, &char_mux_type_info },
> > -        { &memory_driver, &char_memory_type_info }
> > -    };
> > -    int i;
> > -
> > -    type_register_static(&char_type_info);
> > -#ifndef _WIN32
> > -    type_register_static(&char_fd_type_info);
> > -#else
> > -    type_register_static(&char_win_type_info);
> > -    type_register_static(&char_win_stdio_type_info);
> > -#endif
> 
> ...when they used to be last; but I don't think it hurts.

It doesn't, and it is spread later in the series, I wouldn't bother.

> 
> > -    for (i = 0; i < ARRAY_SIZE(chardevs); i++) {
> > -        type_register_static(chardevs[i].type);
> > -        register_char_driver(chardevs[i].driver);
> > -    }
> > +    type_register_static(&char_pipe_type_info);
> > +    type_register_static(&char_mux_type_info);
> > +    type_register_static(&char_memory_type_info);
> >  
> >      /* this must be done after machine init, since we register FEs with
> >      muxes
> >       * as part of realize functions like serial_isa_realizefn when
> >       -nographic
> > diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> 
> Enough bugs that I'll want to see a v2, but it's headed in the right
> direction.

thanks, I'll wait for Paolo to take the first series before sending v2. Feel free to continue review of v1 for the remaining patches.

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