Re: [Qemu-devel] [PATCH 18/40] char: remove class kind field

[Eric Blake <eblake@redhat.com>]

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

On 01/11/2017 11:29 AM, Marc-André Lureau wrote:
> The class kind is necessary to lookup the chardev name in
> qmp_chardev_add() after calling qemu_chr_new_from_opts() and to set
> the appropriate ChardevBackend (mainly to free the right
> fields).
> 
> qemu_chr_new_from_opts() can be changed to use a non-qmp function
> using the chardev class typename. Introduce qemu_chardev_add() to be
> called from qemu_chr_new_from_opts() and remove the class chardev kind
> field. Set the backend->type in the parse callback (when non-common
> fields are added).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---

>  
> +static Chardev *qemu_chardev_add(const char *id, const char *typename,
> +                                 ChardevBackend *backend, Error **errp)
> +{
> +    Chardev *chr;
> +
> +    chr = qemu_chr_find(id);
> +    if (chr) {
> +        error_setg(errp, "Chardev '%s' already exists", id);
> +        return NULL;
> +    }
> +
> +    chr = qemu_chardev_new(id, typename, backend, errp);
> +    if (!chr) {
> +        return NULL;
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> +    return chr;
> +}
> +

This part seems okay.

> @@ -4222,22 +4235,22 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>  
>      cc = char_get_class(name, errp);
>      if (cc == NULL) {
> -        goto err;
> +        return NULL;
>      }
>  
>      backend = g_new0(ChardevBackend, 1);
> +    backend->type = CHARDEV_BACKEND_KIND_NULL;
>  
>      if (qemu_opt_get_bool(opts, "mux", 0)) {
>          bid = g_strdup_printf("%s-base", id);
>      }
>  
>      chr = NULL;
> -    backend->type = cc->kind;

I'm not sure I follow this hunk - we used to set backend->type
dynamically and now it is forced to KIND_NULL.  Is the point that we
don't need to set backend->type here because the later call to
qemu_chardev_add()...

> @@ -4245,37 +4258,33 @@ Chardev *qemu_chr_new_from_opts(QemuOpts *opts,
>          backend->u.null.data = ccom; /* Any ChardevCommon member would work */
>      }
>  
> -    ret = qmp_chardev_add(bid ? bid : id, backend, errp);
> -    if (!ret) {
> -        goto qapi_out;
> +    chr = qemu_chardev_add(bid ? bid : id,
> +                           object_class_get_name(OBJECT_CLASS(cc)),
> +                           backend, errp);

...now passes the Object type which can be used to derive the same same
information?  In that case, was the assignment to backend->type =
KIND_NULL dead?

> +    if (chr == NULL) {
> +        goto out;
>      }
>  
>      if (bid) {
> +        Chardev *mux;
>          qapi_free_ChardevBackend(backend);
> -        qapi_free_ChardevReturn(ret);
>          backend = g_new0(ChardevBackend, 1);
> -        backend->u.mux.data = g_new0(ChardevMux, 1);
>          backend->type = CHARDEV_BACKEND_KIND_MUX;
> +        backend->u.mux.data = g_new0(ChardevMux, 1);

Why the churn on the assignment to backend->u.mux.data?

>          backend->u.mux.data->chardev = g_strdup(bid);
> -        ret = qmp_chardev_add(id, backend, errp);
> -        if (!ret) {
> -            chr = qemu_chr_find(bid);
> +        mux = qemu_chardev_add(id, TYPE_CHARDEV_MUX, backend, errp);
> +        if (mux == NULL) {
>              qemu_chr_delete(chr);
>              chr = NULL;
> -            goto qapi_out;
> +            goto out;
>          }
> +        chr = mux;
>      }
>  
> -    chr = qemu_chr_find(id);
> -
> -qapi_out:
> +out:
>      qapi_free_ChardevBackend(backend);
> -    qapi_free_ChardevReturn(ret);
>      g_free(bid);
>      return chr;
> -
> -err:
> -    return NULL;
>  }
>  

> @@ -5010,24 +5014,18 @@ Chardev *qemu_chardev_new(const char *id, const char *typename,
>  ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
>                                 Error **errp)
>  {
> -    const ChardevClass *cc;
>      ChardevReturn *ret;
> +    const ChardevClass *cc;

Why the churn on this declaration?

>      Chardev *chr;
>  
> -    chr = qemu_chr_find(id);
> -    if (chr) {
> -        error_setg(errp, "Chardev '%s' already exists", id);
> -        return NULL;
> -    }
> -
>      cc = char_get_class(ChardevBackendKind_lookup[backend->type], errp);
> -    if (!cc) {
> +    if (cc == NULL) {

Why the churn on this conditional?

>          return NULL;
>      }
>  
> -    chr = qemu_chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> +    chr = qemu_chardev_add(id, object_class_get_name(OBJECT_CLASS(cc)),
>                             backend, errp);
> -    if (!chr) {
> +    if (chr == NULL) {

and again


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