[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
> - { ¶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
- [Qemu-devel] [PATCH 13/40] char-win: convert to finalize, (continued)
- [Qemu-devel] [PATCH 13/40] char-win: convert to finalize, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 11/40] char-win-stdio: convert to finalize, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 14/40] char-fd: convert to finalize, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 12/40] char-win: do not override chr_free, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 15/40] char: remove chr_free, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver, Marc-André Lureau, 2017/01/11
- Re: [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver,
Eric Blake <=
- [Qemu-devel] [PATCH 17/40] char: rename remaining CharDriver to Chardev, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 18/40] char: remove class kind field, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 21/40] char: make null_chr_write() the default method, Marc-André Lureau, 2017/01/11
- [Qemu-devel] [PATCH 22/40] char: move null chardev to its own file, Marc-André Lureau, 2017/01/11