[PATCH v7 0/4] VNC-related HMP/QMP fixes

Stefan Reiter posted 4 patches 2 years, 5 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
docs/about/deprecated.rst  |   6 ++
hmp-commands.hx            |  24 +++---
monitor/hmp-cmds.c         |  48 +++++++++++-
monitor/hmp.c              |  19 ++++-
monitor/monitor-internal.h |   3 +-
monitor/qmp-cmds.c         |  54 ++++---------
qapi/ui.json               | 156 ++++++++++++++++++++++++++++++++-----
7 files changed, 236 insertions(+), 74 deletions(-)
[PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Stefan Reiter 2 years, 5 months ago
Since the removal of the generic 'qmp_change' command, one can no longer replace
the 'default' VNC display listen address at runtime (AFAIK). For our users who
need to set up a secondary VNC access port, this means configuring a second VNC
display (in addition to our standard one for web-access), but it turns out one
cannot set a password on this second display at the moment, as the
'set_password' call only operates on the 'default' display.

Additionally, using secret objects, the password is only read once at startup.
This could be considered a bug too, but is not touched in this series and left
for a later date.

v6 -> v7:
* remove g_strdup and g_free, use strings directly
* squash in last patch

v5 -> v6:
* consider feedback from Markus' review, mainly:
  * fix crash bug in patch 1 (sorry, artifact of patch-splitting)
  * rely on '!has_param => param == NULL' to shorten code
  * add note to 'docs/about/deprecated.rst' and touch up comments a bit
* go back to g_free instead of qapi_free_* since the latter apparently tries to
  free the passed in pointer which lives on the stack...
* fix bug in HMP parsing (see patch 1)

v4 -> v5:
* add comment to patch 1 in "monitor-internal.h"
* use qapi_free_SetPasswordOptions and friends, don't leak strdups
* split QAPI change into 3 seperate patches

v3 -> v4:
* drop previously patch 1, this was fixed here instead:
  https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02529.html
* patch 1: add Eric's R-b
* patch 2: remove if-assignment, use 'deprecated' feature in schema

v2 -> v3:
* refactor QMP schema for set/expire_password as suggested by Eric Blake and
  Markus Armbruster

v1 -> v2:
* add Marc-André's R-b on patch 1
* use '-d' flag as suggested by Eric Blake and Gerd Hoffmann
  * I didn't see a way to do this yet, so I added a "flags with values" arg type


Stefan Reiter (4):
  monitor/hmp: add support for flag argument with value
  qapi/monitor: refactor set/expire_password with enums
  qapi/monitor: allow VNC display id in set/expire_password
  qapi/monitor: only allow 'keep' SetPasswordAction for VNC and
    deprecate

 docs/about/deprecated.rst  |   6 ++
 hmp-commands.hx            |  24 +++---
 monitor/hmp-cmds.c         |  48 +++++++++++-
 monitor/hmp.c              |  19 ++++-
 monitor/monitor-internal.h |   3 +-
 monitor/qmp-cmds.c         |  54 ++++---------
 qapi/ui.json               | 156 ++++++++++++++++++++++++++++++++-----
 7 files changed, 236 insertions(+), 74 deletions(-)

-- 
2.30.2


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 5 months ago
I'm done reviewing.  David, care to have another look at the HMP part?


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Dr. David Alan Gilbert 2 years, 5 months ago
* Markus Armbruster (armbru@redhat.com) wrote:
> I'm done reviewing.  David, care to have another look at the HMP part?

Yep, looking good to me - is that going via qmp, hmp, or vnc ?

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 5 months ago
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> I'm done reviewing.  David, care to have another look at the HMP part?
>
> Yep, looking good to me - is that going via qmp, hmp, or vnc ?

Either is fine with me.

David, Gerd, do you have anything queued up already?


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Gerd Hoffmann 2 years, 5 months ago
On Tue, Oct 26, 2021 at 01:32:29PM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> I'm done reviewing.  David, care to have another look at the HMP part?
> >
> > Yep, looking good to me - is that going via qmp, hmp, or vnc ?
> 
> Either is fine with me.
> 
> David, Gerd, do you have anything queued up already?

Nothing queued atm, no objections to someone else picking this up.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 5 months ago
Stefan Reiter <s.reiter@proxmox.com> writes:

> Since the removal of the generic 'qmp_change' command, one can no longer replace
> the 'default' VNC display listen address at runtime (AFAIK). For our users who
> need to set up a secondary VNC access port, this means configuring a second VNC
> display (in addition to our standard one for web-access), but it turns out one
> cannot set a password on this second display at the moment, as the
> 'set_password' call only operates on the 'default' display.
>
> Additionally, using secret objects, the password is only read once at startup.
> This could be considered a bug too, but is not touched in this series and left
> for a later date.

Queued, thanks!


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 5 months ago
Markus Armbruster <armbru@redhat.com> writes:

> Stefan Reiter <s.reiter@proxmox.com> writes:
>
>> Since the removal of the generic 'qmp_change' command, one can no longer replace
>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
>> need to set up a secondary VNC access port, this means configuring a second VNC
>> display (in addition to our standard one for web-access), but it turns out one
>> cannot set a password on this second display at the moment, as the
>> 'set_password' call only operates on the 'default' display.
>>
>> Additionally, using secret objects, the password is only read once at startup.
>> This could be considered a bug too, but is not touched in this series and left
>> for a later date.
>
> Queued, thanks!

Unqueued, because it fails to compile with --disable-vnc and with
--disable-spice.  I failed to catch this in review, sorry.

Making it work takes a tiresome amount of #ifdeffery (sketch appended).
Missing: removal of stubs that are no longer used,
e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
than it's worth.

To maximize our chances to get this into 6.2, please respin without the
'if' conditionals.  Yes, this makes introspection less useful, but it's
no worse than before the patch.


diff --git a/qapi/ui.json b/qapi/ui.json
index 5292617b44..39ca0b5ead 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -69,8 +69,10 @@
   'base': { 'protocol': 'DisplayProtocol',
             'password': 'str' },
   'discriminator': 'protocol',
-  'data': { 'vnc': 'SetPasswordOptionsVnc',
-            'spice': 'SetPasswordOptionsSpice' } }
+  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
+                     'if': 'CONFIG_VNC' },
+            'spice': { 'type': 'SetPasswordOptionsSpice',
+                       'if': 'CONFIG_SPICE' } } }
 
 ##
 # @SetPasswordOptionsSpice:
@@ -155,7 +157,8 @@
   'base': { 'protocol': 'DisplayProtocol',
             'time': 'str' },
   'discriminator': 'protocol',
-  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
+  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
+                     'if': 'CONFIG_VNC' } } }
 
 ##
 # @ExpirePasswordOptionsVnc:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f0f0c82d59..f714b2d316 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,24 +1451,40 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
     const char *password  = qdict_get_str(qdict, "password");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
     const char *display = qdict_get_try_str(qdict, "display");
+#endif
+#ifdef CONFIG_SPICE
     const char *connected = qdict_get_try_str(qdict, "connected");
+#endif
     Error *err = NULL;
+    int proto;
 
     SetPasswordOptions opts = {
         .password = (char *)password,
     };
 
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
     if (err) {
         goto out;
     }
 
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+    switch (proto) {
+#ifdef CONFIG_VNC
+    case -1:
+        proto = DISPLAY_PROTOCOL_VNC;
+        /* fall through */
+    case DISPLAY_PROTOCOL_VNC:
         opts.u.vnc.has_display = !!display;
         opts.u.vnc.display = (char *)display;
-    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+        break;
+#else
+    case -1:
+        error_setg(&err, "FIXME");
+        goto out;
+#endif
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         opts.u.spice.has_connected = !!connected;
         opts.u.spice.connected =
             qapi_enum_parse(&SetPasswordAction_lookup, connected,
@@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
         if (err) {
             goto out;
         }
+        break;
+#endif
+    default:
+        ;
     }
 
+    opts.protocol = proto;
     qmp_set_password(&opts, &err);
 
 out:
@@ -1488,22 +1509,34 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
     const char *whenstr = qdict_get_str(qdict, "time");
+#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
     const char *display = qdict_get_try_str(qdict, "display");
+#endif
     Error *err = NULL;
+    int proto;
 
     ExpirePasswordOptions opts = {
         .time = (char *)whenstr,
     };
 
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
+    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
     if (err) {
         goto out;
     }
 
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+    switch (proto) {
+#ifdef CONFIG_VNC
+    case -1:
+        proto = DISPLAY_PROTOCOL_VNC;
+        /* fall through */
+    case DISPLAY_PROTOCOL_VNC:
         opts.u.vnc.has_display = !!display;
         opts.u.vnc.display = (char *)display;
+#else
+    case -1:
+        error_setg(&err, "FIXME");
+        goto out;
+#endif
     }
 
     qmp_expire_password(&opts, &err);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 4825d0cbea..69a9c2977a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
 {
     int rc = 0;
 
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+    switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice.set_passwd(opts->password,
                 opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL,
                 opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
-    } else {
-        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        break;
+#endif
+#ifdef CONFIG_VNC
+    case DISPLAY_PROTOCOL_VNC:
         /* Note that setting an empty password will not disable login through
          * this interface. */
         rc = vnc_display_password(opts->u.vnc.display, opts->password);
+        break;
+#endif
+    default:
+        abort();
     }
 
     if (rc != 0) {
@@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
         when = strtoull(whenstr, NULL, 10);
     }
 
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+    switch (opts->protocol) {
+#ifdef CONFIG_SPICE
+    case DISPLAY_PROTOCOL_SPICE:
         if (!qemu_using_spice(errp)) {
             return;
         }
         rc = qemu_spice.set_pw_expire(when);
-    } else {
-        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        break;
+#endif
+#ifdef CONFIG_VNC
+    case DISPLAY_PROTOCOL_VNC:
         rc = vnc_display_pw_expire(opts->u.vnc.display, when);
+        break;
+#endif
+    default:
+        abort();
     }
 
     if (rc != 0) {


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Fabian Ebner 2 years, 2 months ago
Am 28.10.21 um 21:37 schrieb Markus Armbruster:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>
>>> Since the removal of the generic 'qmp_change' command, one can no longer replace
>>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
>>> need to set up a secondary VNC access port, this means configuring a second VNC
>>> display (in addition to our standard one for web-access), but it turns out one
>>> cannot set a password on this second display at the moment, as the
>>> 'set_password' call only operates on the 'default' display.
>>>
>>> Additionally, using secret objects, the password is only read once at startup.
>>> This could be considered a bug too, but is not touched in this series and left
>>> for a later date.
>>
>> Queued, thanks!
> 
> Unqueued, because it fails to compile with --disable-vnc and with
> --disable-spice.  I failed to catch this in review, sorry.
> 
> Making it work takes a tiresome amount of #ifdeffery (sketch appended).
> Missing: removal of stubs that are no longer used,
> e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
> than it's worth.
> 
> To maximize our chances to get this into 6.2, please respin without the
> 'if' conditionals.  Yes, this makes introspection less useful, but it's
> no worse than before the patch.
> 

Unfortunately, Stefan is no longer working with Proxmox, so I would pick 
up these patches instead.

Since the 6.2 ship has long sailed, I suppose the way forward is using 
the #ifdefs then?

 From my understanding what should be done is:

* In the first patch, fix the typo spotted by Eric Blake and add the R-b 
tags from this round.

* Replace "since 6.2" with "since 7.0" everywhere.

* Incorporate the #ifdef handling from below. I had to add another one 
for the when/whenstr handling in qmp_expire_password to avoid an error 
with -Werror=unused-but-set-variable.

* Add #ifdefs for the unused stubs too? If yes, how to best find them?

> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5292617b44..39ca0b5ead 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -69,8 +69,10 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'password': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
> -            'spice': 'SetPasswordOptionsSpice' } }
> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' },
> +            'spice': { 'type': 'SetPasswordOptionsSpice',
> +                       'if': 'CONFIG_SPICE' } } }
>   
>   ##
>   # @SetPasswordOptionsSpice:
> @@ -155,7 +157,8 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'time': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' } } }
>   
>   ##
>   # @ExpirePasswordOptionsVnc:
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index f0f0c82d59..f714b2d316 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1451,24 +1451,40 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>   {
>       const char *protocol  = qdict_get_str(qdict, "protocol");
>       const char *password  = qdict_get_str(qdict, "password");
> +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>       const char *display = qdict_get_try_str(qdict, "display");
> +#endif
> +#ifdef CONFIG_SPICE
>       const char *connected = qdict_get_try_str(qdict, "connected");
> +#endif
>       Error *err = NULL;
> +    int proto;
>   
>       SetPasswordOptions opts = {
>           .password = (char *)password,
>       };
>   
> -    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                                    DISPLAY_PROTOCOL_VNC, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
>       if (err) {
>           goto out;
>       }
>   
> -    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +    switch (proto) {
> +#ifdef CONFIG_VNC
> +    case -1:
> +        proto = DISPLAY_PROTOCOL_VNC;
> +        /* fall through */
> +    case DISPLAY_PROTOCOL_VNC:
>           opts.u.vnc.has_display = !!display;
>           opts.u.vnc.display = (char *)display;
> -    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
> +        break;
> +#else
> +    case -1:
> +        error_setg(&err, "FIXME");
> +        goto out;
> +#endif
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           opts.u.spice.has_connected = !!connected;
>           opts.u.spice.connected =
>               qapi_enum_parse(&SetPasswordAction_lookup, connected,
> @@ -1476,8 +1492,13 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
>           if (err) {
>               goto out;
>           }
> +        break;
> +#endif
> +    default:
> +        ;
>       }
>   
> +    opts.protocol = proto;
>       qmp_set_password(&opts, &err);
>   
>   out:
> @@ -1488,22 +1509,34 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
>   {
>       const char *protocol  = qdict_get_str(qdict, "protocol");
>       const char *whenstr = qdict_get_str(qdict, "time");
> +#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>       const char *display = qdict_get_try_str(qdict, "display");
> +#endif
>       Error *err = NULL;
> +    int proto;
>   
>       ExpirePasswordOptions opts = {
>           .time = (char *)whenstr,
>       };
>   
> -    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> -                                    DISPLAY_PROTOCOL_VNC, &err);
> +    proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol, -1, &err);
>       if (err) {
>           goto out;
>       }
>   
> -    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
> +    switch (proto) {
> +#ifdef CONFIG_VNC
> +    case -1:
> +        proto = DISPLAY_PROTOCOL_VNC;
> +        /* fall through */
> +    case DISPLAY_PROTOCOL_VNC:
>           opts.u.vnc.has_display = !!display;
>           opts.u.vnc.display = (char *)display;
> +#else
> +    case -1:
> +        error_setg(&err, "FIXME");
> +        goto out;
> +#endif
>       }
>   
>       qmp_expire_password(&opts, &err);
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 4825d0cbea..69a9c2977a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>   {
>       int rc = 0;
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_passwd(opts->password,
>                   opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL,
>                   opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
>           rc = vnc_display_password(opts->u.vnc.display, opts->password);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_pw_expire(when);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           rc = vnc_display_pw_expire(opts->u.vnc.display, when);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> 
> 
> 


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 2 months ago
Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>>
>>>> Since the removal of the generic 'qmp_change' command, one can no longer replace
>>>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
>>>> need to set up a secondary VNC access port, this means configuring a second VNC
>>>> display (in addition to our standard one for web-access), but it turns out one
>>>> cannot set a password on this second display at the moment, as the
>>>> 'set_password' call only operates on the 'default' display.
>>>>
>>>> Additionally, using secret objects, the password is only read once at startup.
>>>> This could be considered a bug too, but is not touched in this series and left
>>>> for a later date.
>>>
>>> Queued, thanks!
>> 
>> Unqueued, because it fails to compile with --disable-vnc and with
>> --disable-spice.  I failed to catch this in review, sorry.
>>
>> Making it work takes a tiresome amount of #ifdeffery (sketch appended).
>> Missing: removal of stubs that are no longer used,
>> e.g. vnc_display_password() in ui/vnc-stubs.c.  Feels like more trouble
>> than it's worth.
>> 
>> To maximize our chances to get this into 6.2, please respin without the
>> 'if' conditionals.  Yes, this makes introspection less useful, but it's
>> no worse than before the patch.
>
> Unfortunately, Stefan is no longer working with Proxmox, so I would
> pick up these patches instead.

Appreciated!

> Since the 6.2 ship has long sailed, I suppose the way forward is using
> the #ifdefs then?

Maybe.

We can choose to improve introspection: keep the 'if' conditionals, and
fix the C code to compile with --disable-vnc and --disable-spice.

Or we can leave it imperfect as it was: drop the 'if' conditionals.

If we had a concrete need for better introspection here, the choice
would be easy.  But as far as I know, we don't.  Is better introspection
worth the additional work anyway?  Since you're volunteering to do the
work, you get to decide :)

> From my understanding what should be done is:
>
> * In the first patch, fix the typo spotted by Eric Blake and add the
>   R-b tags from this round.
>
> * Replace "since 6.2" with "since 7.0" everywhere.
>
> * Incorporate the #ifdef handling from below. I had to add another one
>   for the when/whenstr handling in qmp_expire_password to avoid an
>  error with -Werror=unused-but-set-variable.
>
> * Add #ifdefs for the unused stubs too? If yes, how to best find them?

For every call you put under #if, check whether there are are any
unconditional calls left, and if not, whether there is a stub function
for it.  If this is too terse, just ask for more help.


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Fabian Ebner 2 years, 2 months ago
Am 28.10.21 um 21:37 schrieb Markus Armbruster:
> Markus Armbruster <armbru@redhat.com> writes:
> 

----8<----

> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 5292617b44..39ca0b5ead 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -69,8 +69,10 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'password': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
> -            'spice': 'SetPasswordOptionsSpice' } }
> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' },
> +            'spice': { 'type': 'SetPasswordOptionsSpice',
> +                       'if': 'CONFIG_SPICE' } } }
>   
>   ##
>   # @SetPasswordOptionsSpice:
> @@ -155,7 +157,8 @@
>     'base': { 'protocol': 'DisplayProtocol',
>               'time': 'str' },
>     'discriminator': 'protocol',
> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
> +                     'if': 'CONFIG_VNC' } } }
>   

So I decided to give the #ifdef approach a go, but if I configure with 
--disable-spice --disable-vnc, even with the above conditionals, it is 
still be possible to issue a set_password qmp command, which will then 
lead to an abort() in the generated code (and the patched 
qmp_set_password below would do the same if it could be reached):

Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
#2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members 
(v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
#3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized 
out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
qapi/qapi-commands-ui.c:49
#4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0) at 
../qapi/qmp-dispatch.c:129
#5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at 
../util/async.c:141

Is that expected? Should I add a conditional for {set,expire}_password 
in the schema too, and add an
#if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
around the whole {hmp,qmp}_{set,expire}_password functions/declarations 
in C?

Or maybe that's a good indication that it's really not worth it ;)?

P.S. Thank you for the QAPI-related explanation in the other mail!

----8<----

> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 4825d0cbea..69a9c2977a 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -167,18 +167,26 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
>   {
>       int rc = 0;
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_passwd(opts->password,
>                   opts->u.spice.connected == SET_PASSWORD_ACTION_FAIL,
>                   opts->u.spice.connected == SET_PASSWORD_ACTION_DISCONNECT);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           /* Note that setting an empty password will not disable login through
>            * this interface. */
>           rc = vnc_display_password(opts->u.vnc.display, opts->password);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> @@ -202,14 +210,22 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
>           when = strtoull(whenstr, NULL, 10);
>       }
>   
> -    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
> +    switch (opts->protocol) {
> +#ifdef CONFIG_SPICE
> +    case DISPLAY_PROTOCOL_SPICE:
>           if (!qemu_using_spice(errp)) {
>               return;
>           }
>           rc = qemu_spice.set_pw_expire(when);
> -    } else {
> -        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
> +        break;
> +#endif
> +#ifdef CONFIG_VNC
> +    case DISPLAY_PROTOCOL_VNC:
>           rc = vnc_display_pw_expire(opts->u.vnc.display, when);
> +        break;
> +#endif
> +    default:
> +        abort();
>       }
>   
>       if (rc != 0) {
> 
> 
> 


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 2 months ago
Fabian Ebner <f.ebner@proxmox.com> writes:

> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>
> ----8<----
>
>> diff --git a/qapi/ui.json b/qapi/ui.json
>> index 5292617b44..39ca0b5ead 100644
>> --- a/qapi/ui.json
>> +++ b/qapi/ui.json
>> @@ -69,8 +69,10 @@
>>     'base': { 'protocol': 'DisplayProtocol',
>>               'password': 'str' },
>>     'discriminator': 'protocol',
>> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
>> -            'spice': 'SetPasswordOptionsSpice' } }
>> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
>> +                     'if': 'CONFIG_VNC' },
>> +            'spice': { 'type': 'SetPasswordOptionsSpice',
>> +                       'if': 'CONFIG_SPICE' } } }
>>     ##
>>   # @SetPasswordOptionsSpice:
>> @@ -155,7 +157,8 @@
>>     'base': { 'protocol': 'DisplayProtocol',
>>               'time': 'str' },
>>     'discriminator': 'protocol',
>> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
>> +                     'if': 'CONFIG_VNC' } } }
>>   
>
> So I decided to give the #ifdef approach a go, but if I configure with
> --disable-spice --disable-vnc, even with the above conditionals, it is 
> still be possible to issue a set_password qmp command, which will then
> lead to an abort() in the generated code (and the patched 
> qmp_set_password below would do the same if it could be reached):
>
> Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
> #1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
> #2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members
>  (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
> errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
> #3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized
>  out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
> qapi/qapi-commands-ui.c:49
> #4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0)
>  at ../qapi/qmp-dispatch.c:129
> #5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at
>  ../util/async.c:141
>
> Is that expected? Should I add a conditional for {set,expire}_password
> in the schema too, and add an
> #if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
> around the whole {hmp,qmp}_{set,expire}_password
> functions/declarations in C?

I can have a closer look.  To make it easy, tell me how I can pull your
patches, or, if that's inconvenient for you, send them to me.

> Or maybe that's a good indication that it's really not worth it ;)?

Possibly.

> P.S. Thank you for the QAPI-related explanation in the other mail!

You're welcome!


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 2 months ago
Markus Armbruster <armbru@redhat.com> writes:

> Fabian Ebner <f.ebner@proxmox.com> writes:
>
>> Am 28.10.21 um 21:37 schrieb Markus Armbruster:
>>> Markus Armbruster <armbru@redhat.com> writes:
>>> 
>>
>> ----8<----
>>
>>> diff --git a/qapi/ui.json b/qapi/ui.json
>>> index 5292617b44..39ca0b5ead 100644
>>> --- a/qapi/ui.json
>>> +++ b/qapi/ui.json
>>> @@ -69,8 +69,10 @@
>>>     'base': { 'protocol': 'DisplayProtocol',
>>>               'password': 'str' },
>>>     'discriminator': 'protocol',
>>> -  'data': { 'vnc': 'SetPasswordOptionsVnc',
>>> -            'spice': 'SetPasswordOptionsSpice' } }
>>> +  'data': { 'vnc': { 'type': 'SetPasswordOptionsVnc',
>>> +                     'if': 'CONFIG_VNC' },
>>> +            'spice': { 'type': 'SetPasswordOptionsSpice',
>>> +                       'if': 'CONFIG_SPICE' } } }
>>>     ##
>>>   # @SetPasswordOptionsSpice:
>>> @@ -155,7 +157,8 @@
>>>     'base': { 'protocol': 'DisplayProtocol',
>>>               'time': 'str' },
>>>     'discriminator': 'protocol',
>>> -  'data': { 'vnc': 'ExpirePasswordOptionsVnc' } }
>>> +  'data': { 'vnc': { 'type': 'ExpirePasswordOptionsVnc',
>>> +                     'if': 'CONFIG_VNC' } } }
>>>   
>>
>> So I decided to give the #ifdef approach a go, but if I configure with
>> --disable-spice --disable-vnc, even with the above conditionals, it is 
>> still be possible to issue a set_password qmp command, which will then
>> lead to an abort() in the generated code (and the patched 
>> qmp_set_password below would do the same if it could be reached):
>>
>> Thread 1 (Thread 0x7f4a86701ec0 (LWP 40487) "qemu-system-x86"):
>> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
>> #1  0x00007f4a90d72537 in __GI_abort () at abort.c:79
>> #2  0x00005583ca03cef3 in visit_type_SetPasswordOptions_members
>>  (v=v@entry=0x5583cc6cccc0, obj=obj@entry=0x7ffe5bfc3ee0, 
>> errp=errp@entry=0x0) at qapi/qapi-visit-ui.c:71
>> #3  0x00005583ca5df72f in qmp_marshal_set_password (args=<optimized
>>  out>, ret=<optimized out>, errp=0x7f4a85d96ea0) at 
>> qapi/qapi-commands-ui.c:49
>> #4  0x00005583ca5e89e9 in do_qmp_dispatch_bh (opaque=0x7f4a85d96eb0)
>>  at ../qapi/qmp-dispatch.c:129
>> #5  0x00005583ca605494 in aio_bh_call (bh=0x7f4a78009270) at
>>  ../util/async.c:141
>>
>> Is that expected? Should I add a conditional for {set,expire}_password
>> in the schema too, and add an
>> #if defined(CONFIG_SPICE) || defined(CONFIG_VNC)
>> around the whole {hmp,qmp}_{set,expire}_password
>> functions/declarations in C?
>
> I can have a closer look.  To make it easy, tell me how I can pull your
> patches, or, if that's inconvenient for you, send them to me.

I got them by e-mail, thanks!

The dealloc visitor for unions (here: SetPasswordOptions) falls apart
when the tag enum (here: DisplayProtocol) is effectively empty.

The dealloc visitor's job is to recursively free a QAPI object.  Unlike
the other visitors, the dealloc visitor needs to work on
half-constructed objects where parts are still zero.  Easy, because
freeing null pointers does nothing.

Complication: for a union, the common visitor core still needs to decide
which branch to enter.  If we never constructed the union's tag value,
it's zero, and so is the branch corresponding to the zero tag value.
The visitor core happily goes down that branch, and the dealloc visitor
happily does nothing for it.  Not exactly the cleanest solution ever,
but it works.

*Except* when the tag enum is empty.  Then we run into the abort() here:

    bool visit_type_SetPasswordOptions_members(Visitor *v, SetPasswordOptions *obj, Error **errp)
    {
        if (!visit_type_q_obj_SetPasswordOptions_base_members(v, (q_obj_SetPasswordOptions_base *)obj, errp)) {
            return false;
        }
        switch (obj->protocol) {
    #if defined(CONFIG_VNC)
        case DISPLAY_PROTOCOL_VNC:
            return visit_type_SetPasswordOptionsVnc_members(v, &obj->u.vnc, errp);
    #endif /* defined(CONFIG_VNC) */
    #if defined(CONFIG_SPICE)
        case DISPLAY_PROTOCOL_SPICE:
            return visit_type_SetPasswordOptionsSpice_members(v, &obj->u.spice, errp);
    #endif /* defined(CONFIG_SPICE) */
        default:
            abort();
        }
        return true;
    }

This is actually why the QAPI generator rejects a union with an empty
tag enum (test case tests/qapi-schema/union-empty.json): it's a
restriction to protect the dealloc visitor.

The QAPI generator doesn't reject a union with a tag enum where all
members are conditional.  Hole in the restriction.

We can either plug the hole in the restriction, or lift the restriction.

>> Or maybe that's a good indication that it's really not worth it ;)?
>
> Possibly.

Let's drop the 'if' conditionals in the interest of decoupling this
series from the QAPI infrastructure fixes needed to make them work.

But first see my question about display-reload upthread.

>> P.S. Thank you for the QAPI-related explanation in the other mail!
>
> You're welcome!


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Markus Armbruster 2 years, 2 months ago
Stefan Reiter <s.reiter@proxmox.com> writes:

> Since the removal of the generic 'qmp_change' command, one can no longer replace
> the 'default' VNC display listen address at runtime (AFAIK). For our users who
> need to set up a secondary VNC access port, this means configuring a second VNC
> display (in addition to our standard one for web-access), but it turns out one
> cannot set a password on this second display at the moment, as the
> 'set_password' call only operates on the 'default' display.
>
> Additionally, using secret objects, the password is only read once at startup.
> This could be considered a bug too, but is not touched in this series and left
> for a later date.

Related: Vladimir recently posted a patch to add a new command for
changing VNC server listening addresses.  Daniel asked him to work it
into display-reload instead[1].  Vladimir complied[2].

Daniel, what do you think about this one?  Should it also use
display-reload?


[1] Message-ID: <YdRJ06CS4qoDJI/F@redhat.com>
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg00292.html

[2] Message-ID: <20220118160909.2502374-1-vsementsov@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg03864.html


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Daniel P. Berrangé 2 years, 2 months ago
On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
> Stefan Reiter <s.reiter@proxmox.com> writes:
> 
> > Since the removal of the generic 'qmp_change' command, one can no longer replace
> > the 'default' VNC display listen address at runtime (AFAIK). For our users who
> > need to set up a secondary VNC access port, this means configuring a second VNC
> > display (in addition to our standard one for web-access), but it turns out one
> > cannot set a password on this second display at the moment, as the
> > 'set_password' call only operates on the 'default' display.
> >
> > Additionally, using secret objects, the password is only read once at startup.
> > This could be considered a bug too, but is not touched in this series and left
> > for a later date.
> 
> Related: Vladimir recently posted a patch to add a new command for
> changing VNC server listening addresses.  Daniel asked him to work it
> into display-reload instead[1].  Vladimir complied[2].
> 
> Daniel, what do you think about this one?  Should it also use
> display-reload?

I'd ultimately intend to deprecate & remove the direct setting of
passwords on the CLI, and exclusively rely on the 'secret' object
for passing in passwords. With this in mind, I'd not be enthusiastic
about adding new commands for changing passwords in QMP directly,
rather I think we should have a way to change the 'secret' object
in use.

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 v7 0/4] VNC-related HMP/QMP fixes
Posted by Fabian Ebner 2 years, 1 month ago
Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé:
> On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
>> Stefan Reiter <s.reiter@proxmox.com> writes:
>>
>>> Since the removal of the generic 'qmp_change' command, one can no longer replace
>>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
>>> need to set up a secondary VNC access port, this means configuring a second VNC
>>> display (in addition to our standard one for web-access), but it turns out one
>>> cannot set a password on this second display at the moment, as the
>>> 'set_password' call only operates on the 'default' display.
>>>
>>> Additionally, using secret objects, the password is only read once at startup.
>>> This could be considered a bug too, but is not touched in this series and left
>>> for a later date.
>>
>> Related: Vladimir recently posted a patch to add a new command for
>> changing VNC server listening addresses.  Daniel asked him to work it
>> into display-reload instead[1].  Vladimir complied[2].
>>
>> Daniel, what do you think about this one?  Should it also use
>> display-reload?
> 
> I'd ultimately intend to deprecate & remove the direct setting of
> passwords on the CLI, and exclusively rely on the 'secret' object
> for passing in passwords. With this in mind, I'd not be enthusiastic
> about adding new commands for changing passwords in QMP directly,
> rather I think we should have a way to change the 'secret' object
> in use.
> 

How should I proceed with this series then? Does adding the new argument
for the display ID count as "adding new commands"?

If what I should do is switching to only using secret objects, would the
plan be something like the following?

1. Add an option to display-reload for switching the display's
password-secret while adding SPICE as a valid display type.
2. Also include the set password action (i.e. disconnect/fail/keep) and
expiration time as part of that option.
3. Extend display-reload to also take an optional display ID for VNC.
4. Deprecate expire_password and set_password.

> Regards,
> Daniel


Re: [PATCH v7 0/4] VNC-related HMP/QMP fixes
Posted by Daniel P. Berrangé 2 years, 1 month ago
On Mon, Jan 31, 2022 at 10:45:08AM +0100, Fabian Ebner wrote:
> Am 25.01.22 um 16:06 schrieb Daniel P. Berrangé:
> > On Mon, Jan 24, 2022 at 02:50:39PM +0100, Markus Armbruster wrote:
> >> Stefan Reiter <s.reiter@proxmox.com> writes:
> >>
> >>> Since the removal of the generic 'qmp_change' command, one can no longer replace
> >>> the 'default' VNC display listen address at runtime (AFAIK). For our users who
> >>> need to set up a secondary VNC access port, this means configuring a second VNC
> >>> display (in addition to our standard one for web-access), but it turns out one
> >>> cannot set a password on this second display at the moment, as the
> >>> 'set_password' call only operates on the 'default' display.
> >>>
> >>> Additionally, using secret objects, the password is only read once at startup.
> >>> This could be considered a bug too, but is not touched in this series and left
> >>> for a later date.
> >>
> >> Related: Vladimir recently posted a patch to add a new command for
> >> changing VNC server listening addresses.  Daniel asked him to work it
> >> into display-reload instead[1].  Vladimir complied[2].
> >>
> >> Daniel, what do you think about this one?  Should it also use
> >> display-reload?
> > 
> > I'd ultimately intend to deprecate & remove the direct setting of
> > passwords on the CLI, and exclusively rely on the 'secret' object
> > for passing in passwords. With this in mind, I'd not be enthusiastic
> > about adding new commands for changing passwords in QMP directly,
> > rather I think we should have a way to change the 'secret' object
> > in use.
> > 
> 
> How should I proceed with this series then? Does adding the new argument
> for the display ID count as "adding new commands"?

Ok, so I've gone back and properly read the series. Since you're simply
extending existing commands with new arguments I've no objection to
carrying on with this approach.

We should still aim to have a general purpose command for live config
changes, but that shouldn't be considered a blocker for you series
here, as your series isn't making the existing situation worse.

> If what I should do is switching to only using secret objects, would the
> plan be something like the following?
> 
> 1. Add an option to display-reload for switching the display's
> password-secret while adding SPICE as a valid display type.
> 2. Also include the set password action (i.e. disconnect/fail/keep) and
> expiration time as part of that option.
> 3. Extend display-reload to also take an optional display ID for VNC.
> 4. Deprecate expire_password and set_password.

I've realized that we shouldn't overload display-reload for dual purposes.

We have two distinct usage scenarios that are meaningul

 - Re-loading the value of the existing secret
 - Changing which secret is used

The 'display-reload' command is reasonable for the first.

We should have a 'display-update' command for the second.

Either way, I don't think this series should be blocked on this since
it is merely modifying an existing command.

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