| Message ID | 20191023173154.30051-5-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau <marcandre.lureau@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.lureau@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?), 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
Hi On Mon, Nov 18, 2019 at 6:12 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau > <marcandre.lureau@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.lureau@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?
diff --git a/chardev/char.c b/chardev/char.c index 7b6b2cb123..e7e7492b0e 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -37,6 +37,7 @@ #include "qemu/help_option.h" #include "qemu/module.h" #include "qemu/option.h" +#include "qemu/id.h" #include "chardev/char-mux.h" @@ -944,10 +945,10 @@ void qemu_chr_set_feature(Chardev *chr, return set_bit(feature, chr->features); } -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); +} + 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); if (!chr_new) { return NULL; } diff --git a/include/qemu/id.h b/include/qemu/id.h index 40c70103e4..b55c406e69 100644 --- a/include/qemu/id.h +++ b/include/qemu/id.h @@ -4,6 +4,7 @@ typedef enum IdSubSystems { ID_QDEV, ID_BLOCK, + ID_CHR, ID_MAX /* last element, used as array size */ } IdSubSystems; diff --git a/util/id.c b/util/id.c index af1c5f1b81..5addb4460e 100644 --- a/util/id.c +++ b/util/id.c @@ -34,6 +34,7 @@ bool id_wellformed(const char *id) static const char *const id_subsys_str[ID_MAX] = { [ID_QDEV] = "qdev", [ID_BLOCK] = "block", + [ID_CHR] = "chr", }; /*
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.lureau@redhat.com> --- chardev/char.c | 32 ++++++++++++++++++++++++-------- include/qemu/id.h | 1 + util/id.c | 1 + 3 files changed, 26 insertions(+), 8 deletions(-)