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 :|