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 <marcandre.lur...@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()...
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
>
>
