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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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 <address@hidden>
> > ---
> 
> >  
> > +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()...

I tried to explain some of the reasons in the commit message.

We need backend->type to be set correctly for the qapi_free_ChardevBackend() to 
work. The kind field used to be on a class member, but we try to get rid of it. 
So we move that information to _parse(), and change the backend type 
appropriately there.

KIND_NULL is the common backend (sharing ChardevCommon).

> 
> > @@ -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?

0 is KIND_FILE, which would lead to invalid free.
 
> > +    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?

fixed

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

sorry, too many rebases, conflicts.. fixed

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