qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver
Date: Wed, 11 Jan 2017 15:33:36 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0

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


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

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

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.

>      }
> +    g_slist_free(l);

Shouldn't this be g_slist_free(list)?

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


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

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

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

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]