Hi On Tue, Aug 3, 2021 at 6:39 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Tue, Aug 03, 2021 at 04:02:29PM +0400, marcandre.lur...@redhat.com > wrote: > > From: Marc-André Lureau <marcandre.lur...@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.lur...@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é <berra...@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 :| > > > -- Marc-André Lureau
