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

[Peter Maydell]

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?),
and qemu_chardev_new() can be called with a NULL id, in
which case it will create one ?

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

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 ?

thanks
-- PMM