| Message ID | 20210803120229.3453699-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | chardev: give some context on chardev-add error | expand |
On Tue, Aug 03, 2021 at 04:02:29PM +0400, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Related to: > https://bugzilla.redhat.com/show_bug.cgi?id=1984721 It is preferrable to describe the problem & approach in the comit message, rather than leaving people to read through countless bug comments to understand it. The original code reported: "attempt to add duplicate property 'char2' to object (type 'container')" Since adding yank support, the current code reports "duplicate yank instance" With this patch applied it now reports: "Failed to add chardev 'char2': duplicate yank instance" This is marginally better, but still not great, not that the original error was great either. It would be nice if we could report "chardev with id 'char2' already exists" > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > chardev/char.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/chardev/char.c b/chardev/char.c > index d959eec522..f59a61774b 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -1031,27 +1031,26 @@ Chardev *qemu_chardev_new(const char *id, const char *typename, > ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, > Error **errp) > { > + ERRP_GUARD(); > const ChardevClass *cc; > ChardevReturn *ret; > - Chardev *chr; > + g_autoptr(Chardev) chr = NULL; > > cc = char_get_class(ChardevBackendKind_str(backend->type), errp); > if (!cc) { > - return NULL; > + goto err; > } > > chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)), > backend, NULL, false, errp); > if (!chr) { > - return NULL; > + goto err; > } > > if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr), > errp)) { > - object_unref(OBJECT(chr)); > - return NULL; > + goto err; > } > - object_unref(OBJECT(chr)); > > ret = g_new0(ChardevReturn, 1); > if (CHARDEV_IS_PTY(chr)) { > @@ -1060,6 +1059,10 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, > } > > return ret; > + > +err: > + error_prepend(errp, "Failed to add chardev '%s': ", id); Lowercase 'f' perhaps ? > + return NULL; > } A weak Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> because it is not quite so bad as current code Regards, Daniel
Hi On Tue, Aug 3, 2021 at 6:39 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Tue, Aug 03, 2021 at 04:02:29PM +0400, marcandre.lureau@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Related to: > > https://bugzilla.redhat.com/show_bug.cgi?id=1984721 > > It is preferrable to describe the problem & approach in the comit > message, rather than leaving people to read through countless bug > comments to understand it. > > I'll update the commit message with your comment. The original code reported: > > "attempt to add duplicate property 'char2' to object (type 'container')" > > > Since adding yank support, the current code reports > > "duplicate yank instance" > > With this patch applied it now reports: > > "Failed to add chardev 'char2': duplicate yank instance" > > This is marginally better, but still not great, not that the original > error was great either. > > It would be nice if we could report > > "chardev with id 'char2' already exists" > I can add a patch for that. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > thanks > --- > > chardev/char.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/chardev/char.c b/chardev/char.c > > index d959eec522..f59a61774b 100644 > > --- a/chardev/char.c > > +++ b/chardev/char.c > > @@ -1031,27 +1031,26 @@ Chardev *qemu_chardev_new(const char *id, const > char *typename, > > ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, > > Error **errp) > > { > > + ERRP_GUARD(); > > const ChardevClass *cc; > > ChardevReturn *ret; > > - Chardev *chr; > > + g_autoptr(Chardev) chr = NULL; > > > > cc = char_get_class(ChardevBackendKind_str(backend->type), errp); > > if (!cc) { > > - return NULL; > > + goto err; > > } > > > > chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)), > > backend, NULL, false, errp); > > if (!chr) { > > - return NULL; > > + goto err; > > } > > > > if (!object_property_try_add_child(get_chardevs_root(), id, > OBJECT(chr), > > errp)) { > > - object_unref(OBJECT(chr)); > > - return NULL; > > + goto err; > > } > > - object_unref(OBJECT(chr)); > > > > ret = g_new0(ChardevReturn, 1); > > if (CHARDEV_IS_PTY(chr)) { > > @@ -1060,6 +1059,10 @@ ChardevReturn *qmp_chardev_add(const char *id, > ChardevBackend *backend, > > } > > > > return ret; > > + > > +err: > > + error_prepend(errp, "Failed to add chardev '%s': ", id); > > Lowercase 'f' perhaps ? > We generally start error messages with an uppercase in this unit, but it's not very consistent. > > + return NULL; > > } > > A weak > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > because it is not quite so bad as current code > > > Regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- > https://www.instagram.com/dberrange :| > > >
diff --git a/chardev/char.c b/chardev/char.c index d959eec522..f59a61774b 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -1031,27 +1031,26 @@ Chardev *qemu_chardev_new(const char *id, const char *typename, ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, Error **errp) { + ERRP_GUARD(); const ChardevClass *cc; ChardevReturn *ret; - Chardev *chr; + g_autoptr(Chardev) chr = NULL; cc = char_get_class(ChardevBackendKind_str(backend->type), errp); if (!cc) { - return NULL; + goto err; } chr = chardev_new(id, object_class_get_name(OBJECT_CLASS(cc)), backend, NULL, false, errp); if (!chr) { - return NULL; + goto err; } if (!object_property_try_add_child(get_chardevs_root(), id, OBJECT(chr), errp)) { - object_unref(OBJECT(chr)); - return NULL; + goto err; } - object_unref(OBJECT(chr)); ret = g_new0(ChardevReturn, 1); if (CHARDEV_IS_PTY(chr)) { @@ -1060,6 +1059,10 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, } return ret; + +err: + error_prepend(errp, "Failed to add chardev '%s': ", id); + return NULL; } ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,