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.lur...@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? > @@ -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 > - { ¶llel_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
signature.asc
Description: OpenPGP digital signature