Re: [PATCH v6 14/27] ui: add proper error reporting for password changes

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

On Wed, Feb 18, 2026 at 01:10:09PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Neither the VNC or SPICE code for password changes provides error
> > reporting at source, leading the callers to report a largely useless
> > generic error message.
> >
> > Fixing this removes one of the two remaining needs for the undesirable
> > error_printf_unless_qmp() method.
> >
> > While fixing this the error message hint is improved to recommend the
> > 'password-secret' option which allows securely passing a password at
> > startup.
> >
> > Reported-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/ui/console.h                 |  2 +-
> >  include/ui/qemu-spice-module.h       |  3 ++-
> >  tests/functional/generic/test_vnc.py |  4 ++--
> >  ui/spice-core.c                      | 25 ++++++++++++++++++-------
> >  ui/spice-module.c                    |  7 ++++---
> >  ui/ui-qmp-cmds.c                     | 19 ++++++-------------
> >  ui/vnc-stubs.c                       |  6 +++---
> >  ui/vnc.c                             | 10 +++++++---
> >  8 files changed, 43 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h
> > index 98feaa58bd..3677a9d334 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -457,7 +457,7 @@ void qemu_display_help(void);
> >  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);
> > +int vnc_display_password(const char *id, const char *password, Error **errp);
> >  int vnc_display_pw_expire(const char *id, time_t expires);
> >  void vnc_parse(const char *str);
> >  int vnc_init_func(void *opaque, QemuOpts *opts, Error **errp);
> > diff --git a/include/ui/qemu-spice-module.h b/include/ui/qemu-spice-module.h
> > index 1f22d557ea..072efa0c83 100644
> > --- a/include/ui/qemu-spice-module.h
> > +++ b/include/ui/qemu-spice-module.h
> > @@ -29,7 +29,8 @@ struct QemuSpiceOps {
> >      void (*display_init)(void);
> >      int (*migrate_info)(const char *h, int p, int t, const char *s);
> >      int (*set_passwd)(const char *passwd,
> > -                      bool fail_if_connected, bool disconnect_if_connected);
> > +                      bool fail_if_connected, bool disconnect_if_connected,
> > +                      Error **errp);
> >      int (*set_pw_expire)(time_t expires);
> >      int (*display_add_client)(int csock, int skipauth, int tls);
> >  #ifdef CONFIG_SPICE
> > diff --git a/tests/functional/generic/test_vnc.py b/tests/functional/generic/test_vnc.py
> > index f1dd1597cf..097f858ca1 100755
> > --- a/tests/functional/generic/test_vnc.py
> > +++ b/tests/functional/generic/test_vnc.py
> > @@ -48,7 +48,7 @@ def test_no_vnc_change_password(self):
> >          self.assertEqual(set_password_response['error']['class'],
> >                           'GenericError')
> >          self.assertEqual(set_password_response['error']['desc'],
> > -                         'Could not set password')
> > +                         'No VNC display is present');
> >  
> >      def launch_guarded(self):
> >          try:
> > @@ -73,7 +73,7 @@ def test_change_password_requires_a_password(self):
> >          self.assertEqual(set_password_response['error']['class'],
> >                           'GenericError')
> >          self.assertEqual(set_password_response['error']['desc'],
> > -                         'Could not set password')
> > +                         'VNC password authentication is disabled')
> >  
> >      def test_change_password(self):
> >          self.set_machine('none')
> > diff --git a/ui/spice-core.c b/ui/spice-core.c
> > index 8a6050f4ae..cdcec34f67 100644
> > --- a/ui/spice-core.c
> > +++ b/ui/spice-core.c
> > @@ -756,7 +756,7 @@ static void qemu_spice_init(void)
> >                               tls_ciphers);
> >      }
> >      if (password) {
> > -        qemu_spice.set_passwd(password, false, false);
> > +        qemu_spice.set_passwd(password, false, false, NULL);
> 
> qemu_spice.set_passwd is qemu_spice_set_passwd().  It's converted to
> Error below.  That conversion doesn't replace error reporting, it only
> adds.  Therefore, passing NULL does not lose error reporting here.
> Good.
> 
> However, why is ignoring errors okay here?  Not this patch's fault, of
> course.

IIUC There are two scenarios qemu_spice_set_passwd can fail

 * The password auth method is not enabled
     -> minor config bug, but we should report that error
 * The fail_if_conn is true and a client is connected
     -> impossible scenario during startup, fine to ignore



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