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

[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.


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) {