[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 16/40] char: get rid of CharDriver |
Date: |
Thu, 12 Jan 2017 11:14:58 -0500 (EST) |
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 <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?
>
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
> > - { ¶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?
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
>
>
- [Qemu-devel] [PATCH 11/40] char-win-stdio: convert to finalize, (continued)
- [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
- [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
- [Qemu-devel] [PATCH 20/40] char: create chardev-obj-y, Marc-André Lureau, 2017/01/11