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

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

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1984721

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);
+    return NULL;
 }
 
 ChardevReturn *qmp_chardev_change(const char *id, ChardevBackend *backend,
-- 
2.32.0.264.g75ae10bc75

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

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

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

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5927 bytes --]