Re: [PATCH 46/60] ui/vnc: merge vnc_display_init() and vnc_display_open()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Mar 17, 2026 at 12:51:00PM +0400, Marc-André Lureau wrote:
> Combine the two-step vnc_display_init()/vnc_display_open() sequence
> into a single vnc_display_new() function that returns VncDisplay*.
> This simplifies the API by making vnc_display_open() an
> internal detail and will allow further code simplification.
> 
> vnc_display_new() is moved to vnc.h, since it returns VncDisplay* now.
> Add vnc_display_free() for consistency, and it will be later used.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/ui/console.h |  2 --
>  ui/vnc.h             |  3 ++
>  ui/vnc.c             | 79 ++++++++++++++++++++++------------------------------
>  3 files changed, 37 insertions(+), 47 deletions(-)
> 
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 152333d60fc..737ceba3890 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -448,8 +448,6 @@ const char *qemu_display_get_vc(DisplayOptions *opts);
>  void qemu_display_help(void);
>  
>  /* vnc.c */
> -void vnc_display_init(const char *id, Error **errp);
> -void vnc_display_open(const char *id, Error **errp);
>  void vnc_display_add_client(const char *id, int csock, bool skipauth);
>  int vnc_display_password(const char *id, const char *password, Error **errp);
>  int vnc_display_pw_expire(const char *id, time_t expires);
> diff --git a/ui/vnc.h b/ui/vnc.h
> index c5d678ac31e..6afe0f16d12 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -548,6 +548,9 @@ enum VncFeatures {
>  #define VNC_CLIPBOARD_NOTIFY   (1 << 27)
>  #define VNC_CLIPBOARD_PROVIDE  (1 << 28)
>  
> +VncDisplay *vnc_display_new(const char *id, Error **errp);
> +void vnc_display_free(VncDisplay *vd);

This re-inforces my comment from the previous patch, wrt stopping
the VNC worker thread. If we're exposing this as a "public" API,
then IMHO, we need to  keep around all the code for stopping the
worker thread, revert 09526058d0a501106dcac842a455e187f1413d98
and actually call vnc_stop_worker_thread in the right place(s).

> +
>  /*****************************************************************************
>   *
>   * Internal APIs
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 115ff8a988e..03b99c9e590 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3421,14 +3421,14 @@ static void vmstate_change_handler(void *opaque, bool running, RunState state)
>      update_displaychangelistener(&vd->dcl, VNC_REFRESH_INTERVAL_BASE);
>  }
>  
> -static void vnc_display_free(VncDisplay *vd);
> +static bool vnc_display_open(VncDisplay *vd, Error **errp);
>  
> -void vnc_display_init(const char *id, Error **errp)
> +VncDisplay *vnc_display_new(const char *id, Error **errp)
>  {
>      VncDisplay *vd;
>  
>      if (vnc_display_find(id) != NULL) {
> -        return;
> +        return NULL;
>      }
>      vd = g_malloc0(sizeof(*vd));
>  
> @@ -3449,7 +3449,7 @@ void vnc_display_init(const char *id, Error **errp)
>  
>      if (!vd->kbd_layout) {
>          vnc_display_free(vd);
> -        return;
> +        return NULL;
>      }
>  
>      vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -3462,7 +3462,13 @@ void vnc_display_init(const char *id, Error **errp)
>      vd->vmstate_handler_entry = qemu_add_vm_change_state_handler(
>          &vmstate_change_handler, vd);
>  
> +    if (!vnc_display_open(vd, errp)) {
> +        vnc_display_free(vd);
> +        return NULL;
> +    }
> +
>      QTAILQ_INSERT_TAIL(&vnc_displays, vd, next);
> +    return vd;
>  }
>  
>  static void vnc_display_close(VncDisplay *vd)
> @@ -3507,7 +3513,7 @@ static void vnc_display_close(VncDisplay *vd)
>  #endif
>  }
>  
> -static void vnc_display_free(VncDisplay *vd)
> +void vnc_display_free(VncDisplay *vd)
>  {
>      if (!vd) {
>          return;
> @@ -3525,7 +3531,6 @@ static void vnc_display_free(VncDisplay *vd)
>      g_free(vd);
>  }
>  
> -
>  int vnc_display_password(const char *id, const char *password, Error **errp)
>  {
>      VncDisplay *vd = vnc_display_find(id);
> @@ -4068,10 +4073,9 @@ bool vnc_display_update(DisplayUpdateOptionsVNC *arg, Error **errp)
>      return true;
>  }
>  
> -void vnc_display_open(const char *id, Error **errp)
> +static bool vnc_display_open(VncDisplay *vd, Error **errp)
>  {
> -    VncDisplay *vd = vnc_display_find(id);
> -    QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, id);
> +    QemuOpts *opts = qemu_opts_find(&qemu_vnc_opts, vd->id);
>      g_autoptr(SocketAddressList) saddr_list = NULL;
>      g_autoptr(SocketAddressList) wsaddr_list = NULL;
>      const char *share, *device_id;
> @@ -4090,26 +4094,23 @@ void vnc_display_open(const char *id, Error **errp)
>      assert(vd);
>      assert(opts);
>  
> -    vnc_display_close(vd);
> -
>      reverse = qemu_opt_get_bool(opts, "reverse", false);
>      if (vnc_display_get_addresses(opts, reverse, &saddr_list, &wsaddr_list,
>                                    errp) < 0) {
> -        goto fail;
> +        return false;
>      }
>  
> -
>      passwordSecret = qemu_opt_get(opts, "password-secret");
>      if (passwordSecret) {
>          if (qemu_opt_get(opts, "password")) {
>              error_setg(errp,
>                         "'password' flag is redundant with 'password-secret'");
> -            goto fail;
> +            return false;
>          }
>          vd->password = qcrypto_secret_lookup_as_utf8(passwordSecret,
>                                                       errp);
>          if (!vd->password) {
> -            goto fail;
> +            return false;
>          }
>          password = true;
>      } else {
> @@ -4120,7 +4121,7 @@ void vnc_display_open(const char *id, Error **errp)
>                  QCRYPTO_CIPHER_ALGO_DES, QCRYPTO_CIPHER_MODE_ECB)) {
>              error_setg(errp,
>                         "Cipher backend does not support DES algorithm");
> -            goto fail;
> +            return false;
>          }
>      }
>  
> @@ -4130,7 +4131,7 @@ void vnc_display_open(const char *id, Error **errp)
>  #ifndef CONFIG_VNC_SASL
>      if (sasl) {
>          error_setg(errp, "VNC SASL auth requires cyrus-sasl support");
> -        goto fail;
> +        return false;
>      }
>  #endif /* CONFIG_VNC_SASL */
>      credid = qemu_opt_get(opts, "tls-creds");
> @@ -4141,7 +4142,7 @@ void vnc_display_open(const char *id, Error **errp)
>          if (!creds) {
>              error_setg(errp, "No TLS credentials with id '%s'",
>                         credid);
> -            goto fail;
> +            return false;
>          }
>          vd->tlscreds = (QCryptoTLSCreds *)
>              object_dynamic_cast(creds,
> @@ -4149,26 +4150,26 @@ void vnc_display_open(const char *id, Error **errp)
>          if (!vd->tlscreds) {
>              error_setg(errp, "Object with id '%s' is not TLS credentials",
>                         credid);
> -            goto fail;
> +            return false;
>          }
>          object_ref(OBJECT(vd->tlscreds));
>  
>          if (!qcrypto_tls_creds_check_endpoint(vd->tlscreds,
>                                                QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
>                                                errp)) {
> -            goto fail;
> +            return false;
>          }
>      }
>      tlsauthz = qemu_opt_get(opts, "tls-authz");
>      if (tlsauthz && !vd->tlscreds) {
>          error_setg(errp, "'tls-authz' provided but TLS is not enabled");
> -        goto fail;
> +        return false;
>      }
>  
>      saslauthz = qemu_opt_get(opts, "sasl-authz");
>      if (saslauthz && !sasl) {
>          error_setg(errp, "'sasl-authz' provided but SASL auth is not enabled");
> -        goto fail;
> +        return false;
>      }
>  
>      share = qemu_opt_get(opts, "share");
> @@ -4181,7 +4182,7 @@ void vnc_display_open(const char *id, Error **errp)
>              vd->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
>          } else {
>              error_setg(errp, "unknown vnc share= option");
> -            goto fail;
> +            return false;
>          }
>      } else {
>          vd->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
> @@ -4215,20 +4216,20 @@ void vnc_display_open(const char *id, Error **errp)
>      if (vnc_display_setup_auth(&vd->auth, &vd->subauth,
>                                 vd->tlscreds, password,
>                                 sasl, false, errp) < 0) {
> -        goto fail;
> +        return false;
>      }
>      trace_vnc_auth_init(vd, 0, vd->auth, vd->subauth);
>  
>      if (vnc_display_setup_auth(&vd->ws_auth, &vd->ws_subauth,
>                                 vd->tlscreds, password,
>                                 sasl, true, errp) < 0) {
> -        goto fail;
> +        return false;
>      }
>      trace_vnc_auth_init(vd, 1, vd->ws_auth, vd->ws_subauth);
>  
>  #ifdef CONFIG_VNC_SASL
>      if (sasl && !vnc_sasl_server_init(errp)) {
> -        goto fail;
> +        return false;
>      }
>  #endif
>      vd->lock_key_sync = lock_key_sync;
> @@ -4241,7 +4242,7 @@ void vnc_display_open(const char *id, Error **errp)
>      if (audiodev) {
>          vd->audio_be = audio_be_by_name(audiodev, errp);
>          if (!vd->audio_be) {
> -            goto fail;
> +            return false;
>          }
>      } else {
>          vd->audio_be = audio_get_default_audio_be(NULL);
> @@ -4255,7 +4256,7 @@ void vnc_display_open(const char *id, Error **errp)
>          con = qemu_console_lookup_by_device_name(device_id, head, &err);
>          if (err) {
>              error_propagate(errp, err);
> -            goto fail;
> +            return false;
>          }
>      } else {
>          con = qemu_console_lookup_default();
> @@ -4271,16 +4272,16 @@ void vnc_display_open(const char *id, Error **errp)
>      qkbd_state_set_delay(vd->kbd, key_delay_ms);
>  
>      if (saddr_list == NULL) {
> -        return;
> +        return true;
>      }
>  
>      if (reverse) {
>          if (vnc_display_connect(vd, saddr_list, wsaddr_list, errp) < 0) {
> -            goto fail;
> +            return false;
>          }
>      } else {
>          if (vnc_display_listen(vd, saddr_list, wsaddr_list, errp) < 0) {
> -            goto fail;
> +            return false;
>          }
>      }
>  
> @@ -4288,11 +4289,7 @@ void vnc_display_open(const char *id, Error **errp)
>          vnc_display_print_local_addr(vd);
>      }
>  
> -    /* Success */
> -    return;
> -
> -fail:
> -    vnc_display_close(vd);
> +    return true;
>  }

I think it would be helpful for clarity if we changed vnc_display_init
and vnc_display_open to have a 'bool' return value in an earlier patch,
then let this patch just focus on the merge without the large
refactoring of return vaules.


>  
>  void vnc_display_add_client(const char *id, int csock, bool skipauth)
> @@ -4348,15 +4345,7 @@ int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp)
>          id = vnc_auto_assign_id(opts);
>      }
>  
> -    vnc_display_init(id, errp);
> -    if (*errp) {
> -        return -1;
> -    }
> -    vnc_display_open(id, errp);
> -    if (*errp) {
> -        return -1;
> -    }
> -    return 0;
> +    return vnc_display_new(id, errp) != NULL ? 0 : -1;
>  }
>  
>  static void vnc_register_config(void)
> 
> -- 
> 2.53.0
> 
> 

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