Re: [PATCH] chardev: give some context on chardev-add error

[Daniel P . Berrangé]

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