1. Patchew
  2. QEMU
  3. [PATCH 00/60] ui: add standalone VNC server over D-Bus
  4. View patch

[PATCH 43/60] ui/vnc: simplify vnc_init_func error handling

Marc-André Lureau posted 60 patches 2 weeks, 6 days ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, John Snow <jsnow@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Jan Kiszka <jan.kiszka@web.de>, Phil Dennis-Jordan <phil@philjordan.eu>, Richard Henderson <richard.henderson@linaro.org>, Helge Deller <deller@gmx.de>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Samuel Tardieu <sam@rfc1149.net>, Igor Mitsyanko <i.mitsyanko@gmail.com>, "Hervé Poussineau" <hpoussin@reactos.org>, Aleksandar Rikalo <arikalo@gmail.com>, Laurent Vivier <laurent@vivier.eu>, Thomas Huth <th.huth+qemu@posteo.eu>, BALATON Zoltan <balaton@eik.bme.hu>, "Michael S. Tsirkin" <mst@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Dmitry Fleytman <dmitry.fleytman@gmail.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony PERARD <anthony@xenproject.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Alistair Francis <alistair@alistair23.me>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Fabiano Rosas <farosas@suse.de>
[PATCH 43/60] ui/vnc: simplify vnc_init_func error handling
Posted by Marc-André Lureau 2 weeks, 6 days ago 
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 605e5117b7f..af5fb3c1551 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -4318,7 +4318,7 @@ void vnc_parse(const char *str)
 
 int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_GUARD();
     char *id = (char *)qemu_opts_id(opts);
 
     if (!id) {
@@ -4326,14 +4326,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
         id = vnc_auto_assign_id(opts);
     }
 
-    vnc_display_init(id, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    vnc_display_init(id, errp);
+    if (*errp) {
         return -1;
     }
-    vnc_display_open(id, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
+    vnc_display_open(id, errp);
+    if (*errp) {
         return -1;
     }
     return 0;

-- 
2.53.0
Re: [PATCH 43/60] ui/vnc: simplify vnc_init_func error handling
Posted by Philippe Mathieu-Daudé 5 days, 11 hours ago 
On 17/3/26 09:50, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/vnc.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Re: [PATCH 43/60] ui/vnc: simplify vnc_init_func error handling
Posted by Daniel P. Berrangé 1 week, 6 days ago 
On Tue, Mar 17, 2026 at 12:50:57PM +0400, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 605e5117b7f..af5fb3c1551 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4318,7 +4318,7 @@ void vnc_parse(const char *str)
>  
>  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>  {
> -    Error *local_err = NULL;
> +    ERRP_GUARD();
>      char *id = (char *)qemu_opts_id(opts);
>  
>      if (!id) {
> @@ -4326,14 +4326,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>          id = vnc_auto_assign_id(opts);
>      }
>  
> -    vnc_display_init(id, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    vnc_display_init(id, errp);
> +    if (*errp) {
>          return -1;
>      }
> -    vnc_display_open(id, &local_err);
> -    if (local_err != NULL) {
> -        error_propagate(errp, local_err);
> +    vnc_display_open(id, errp);
> +    if (*errp) {
>          return -1;
>      }
>      return 0;

Unrelated to this patch, I'm wondering about possible bugs in these
two init functions
:

  void vnc_display_init(const char *id, Error **errp)
  {
    VncDisplay *vd;

    if (vnc_display_find(id) != NULL) {
        return;
    }

Why isn't a vnc_display_find failure a reportable error ? With this
"return", the caller thinks we've successfully initialized the
'vc' struct but we haven't.

And


  void vnc_display_open(const char *id, Error **errp)
  {
    VncDisplay *vd = vnc_display_find(id);
    QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);

    ...snip....

    if (!opts) {
        return;
    }


Shouldn't the failure to find 'opts' be an error, rather than an
successful return ?

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
Re: [PATCH 43/60] ui/vnc: simplify vnc_init_func error handling
Posted by Marc-André Lureau 2 days, 11 hours ago 
Hi

On Tue, Mar 24, 2026 at 6:38 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Mar 17, 2026 at 12:50:57PM +0400, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  ui/vnc.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> >
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 605e5117b7f..af5fb3c1551 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -4318,7 +4318,7 @@ void vnc_parse(const char *str)
> >
> >  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> >  {
> > -    Error *local_err = NULL;
> > +    ERRP_GUARD();
> >      char *id = (char *)qemu_opts_id(opts);
> >
> >      if (!id) {
> > @@ -4326,14 +4326,12 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
> >          id = vnc_auto_assign_id(opts);
> >      }
> >
> > -    vnc_display_init(id, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > +    vnc_display_init(id, errp);
> > +    if (*errp) {
> >          return -1;
> >      }
> > -    vnc_display_open(id, &local_err);
> > -    if (local_err != NULL) {
> > -        error_propagate(errp, local_err);
> > +    vnc_display_open(id, errp);
> > +    if (*errp) {
> >          return -1;
> >      }
> >      return 0;
>
> Unrelated to this patch, I'm wondering about possible bugs in these
> two init functions
> :
>
>   void vnc_display_init(const char *id, Error **errp)
>   {
>     VncDisplay *vd;
>
>     if (vnc_display_find(id) != NULL) {
>         return;
>     }
>
> Why isn't a vnc_display_find failure a reportable error ? With this
> "return", the caller thinks we've successfully initialized the
> 'vc' struct but we haven't.

If "vnc_display_find(id) != NULL", there is already a display
succesfully init/registered with this id.

>
> And
>
>
>   void vnc_display_open(const char *id, Error **errp)
>   {
>     VncDisplay *vd = vnc_display_find(id);
>     QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
>
>     ...snip....
>
>     if (!opts) {
>         return;
>     }
>
>
> Shouldn't the failure to find 'opts' be an error, rather than an
> successful return ?

Yes, see the later patch, "ui/vnc: assert preconditions instead of
silently returning".

thanks

> With regards,
> Daniel
> --
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.