Re: [PATCH v3 04/33] chardev: generate an internal id when none given

[Marc-André Lureau]

Hi

On Mon, Nov 18, 2019 at 6:12 PM Peter Maydell <address@hidden> wrote:
>
> On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau
> <address@hidden> 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 <address@hidden>
>
> > -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?