Hi
On Mon, Nov 18, 2019 at 6:12 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau
> <marcandre.lur...@redhat.com> wrote:
> >
> > Internally, qemu may create chardev without ID. Those will not be
> > looked up with qemu_chr_find(), which prevents using qdev_prop_set_chr().
> >
> > Use id_generate(), to generate an internal name (prefixed with #), so
> > no conflict exist with user-named chardev.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> > -Chardev *qemu_chardev_new(const char *id, const char *typename,
> > - ChardevBackend *backend,
> > - GMainContext *gcontext,
> > - Error **errp)
> > +static Chardev *chardev_new(const char *id, const char *typename,
> > + ChardevBackend *backend,
> > + GMainContext *gcontext,
> > + Error **errp)
> > {
> > Object *obj;
> > Chardev *chr = NULL;
> > @@ -991,6 +992,21 @@ end:
> > return chr;
> > }
> >
> > +Chardev *qemu_chardev_new(const char *id, const char *typename,
> > + ChardevBackend *backend,
> > + GMainContext *gcontext,
> > + Error **errp)
> > +{
> > + g_autofree char *genid = NULL;
> > +
> > + if (!id) {
> > + genid = id_generate(ID_CHR);
> > + id = genid;
> > + }
> > +
> > + return chardev_new(id, typename, backend, gcontext, errp);
> > +}
>
> So presumably the idea is that chardev_new() now must be
> called with a non-NULL id (should it assert() that?),
Not, it can still be called with NULL (mostly for qmp_chardev_change()).
> and qemu_chardev_new() can be called with a NULL id, in
> which case it will create one ?
Right
>
> > +
> > ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> > Error **errp)
> > {
> > @@ -1003,8 +1019,8 @@ ChardevReturn *qmp_chardev_add(const char *id,
> > ChardevBackend *backend,
> > return NULL;
> > }
> >
> > - chr = qemu_chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> > - backend, NULL, errp);
> > + chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)),
> > + backend, NULL, errp);
> > if (!chr) {
> > return NULL;
> > }
> > @@ -1061,8 +1077,8 @@ ChardevReturn *qmp_chardev_change(const char *id,
> > ChardevBackend *backend,
> > return NULL;
> > }
> >
> > - chr_new = qemu_chardev_new(NULL,
> > object_class_get_name(OBJECT_CLASS(cc)),
> > - backend, chr->gcontext, errp);
> > + chr_new = chardev_new(NULL, object_class_get_name(OBJECT_CLASS(cc)),
> > + backend, chr->gcontext, errp);
>
> ...but if that's so, why are we calling chardev_new() here
> and passing a NULL pointer ?
Because it's qmp_chardev_change(), it's a transient state, the id is
set after, and reparenting too. The code could probably be structured
differently.
>
> How many callsites actually pass NULL, anyway? My grep
> seems to show:
> * this qmp_chardev_change() call
> * gdbstub.c
> * hw/bt/hci-csr.c
> * tests/test-char.c
>
> Maybe we should just make them all pass in ID strings instead ?
Well, in this case, we must be sure it doesn't conflict with user ID
(presumably with # prefix), or duplicate etc. Why not leave it to
id_generate() then?
