Re: [Qemu-devel] [PATCH 20/20] chardev: qom-ify

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2772 bytes --]

On 01/05/2017 10:53 AM, Marc-André Lureau wrote:
> Turn Chardev into Object.
> 
> qemu_chr_alloc() is replaced by the qemu_chardev_new() constructor. It
> will call qemu_char_open() to open/intialize the chardev with the
> ChardevCommon *backend settings.
> 
> The CharDriver::create() callback is turned into a ChardevClass::open()
> which is called from the newly introduced qemu_chardev_open().
> 
> "chardev-gdb" and "chardev-hci" are internal chardev and aren't
> creatable directly with -chardev. Use a new internal flag to disable
> them. We may want to use TYPE_USER_CREATABLE interface instead, or
> perhaps allow -chardev usage.
> 
> Although in general we keep typename and macros private, unless the type
> is being used by some other file, in this patch, all types and common
> helper macros for qemu-char.c are in char.h. This is to help transition
> now (some types must be declared early, while some aren't shared) and
> when splitting in several units. This is to be improved later.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

> +++ b/qemu-char.c

> @@ -466,7 +433,8 @@ int qemu_chr_fe_get_msgfds(CharBackend *be, int *fds, int len)
>          return -1;
>      }
>  
> -    return s->driver->get_msgfds ? s->driver->get_msgfds(s, fds, len) : -1;
> +    return CHARDEV_GET_CLASS(s)->get_msgfds ?
> +        CHARDEV_GET_CLASS(s)->get_msgfds(s, fds, len) : -1;

I'm not familiar enough with the Object hierarchy to know if there is
any significant runtime cost to CHARDEV_GET_CLASS() that would warrant
capturing the output in a temporary variable, or if it is simple enough
that a compiler can already figure out that it is a reusable value
rather than computing it twice.  Probably not worrying about, though,
unless it actually shows up in a hot spot on a trace.


> @@ -4957,47 +4994,96 @@ void qemu_chr_set_feature(Chardev *chr,
>      return set_bit(feature, chr->features);
>  }
>  
> -ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> -                               Error **errp)
> +static const ChardevClass *char_get_class(const char *driver, Error **errp)
> +{
> +    ObjectClass *oc;
> +    const ChardevClass *cc;
> +    char *typename = g_strdup_printf("chardev-%s", driver);
> +
> +    oc = object_class_by_name(typename);
> +    g_free(typename);

char_get_class("tty", NULL) will fail, because that's merely an alias
name rather than the canonical "serial" name. Will that matter to any of
the code? I didn't spot anything, so I'm not letting it stop my review.

Reviewed-by: Eric Blake <eblake@redhat.com>

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]