qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_pass


From: Stefan Reiter
Subject: Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password
Date: Thu, 21 Oct 2021 10:42:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0

On 10/21/21 7:05 AM, Markus Armbruster wrote:
Stefan Reiter <s.reiter@proxmox.com> writes:

It is possible to specify more than one VNC server on the command line,
either with an explicit ID or the auto-generated ones à la "default",
"vnc2", "vnc3", ...

It is not possible to change the password on one of these extra VNC
displays though. Fix this by adding a "display" parameter to the
"set_password" and "expire_password" QMP and HMP commands.

For HMP, the display is specified using the "-d" value flag.

For QMP, the schema is updated to explicitly express the supported
variants of the commands with protocol-discriminated unions.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
  hmp-commands.hx    |  24 +++++-----
  monitor/hmp-cmds.c |  51 +++++++++++++++------
  monitor/qmp-cmds.c |  36 ++++++---------
  qapi/ui.json       | 112 +++++++++++++++++++++++++++++++++++----------
  4 files changed, 154 insertions(+), 69 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index cf723c69ac..9fbb207b35 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1514,33 +1514,35 @@ ERST
{
          .name       = "set_password",
-        .args_type  = "protocol:s,password:s,connected:s?",
-        .params     = "protocol password action-if-connected",
+        .args_type  = "protocol:s,password:s,display:-dV,connected:s?",
+        .params     = "protocol password [-d display] [action-if-connected]",
          .help       = "set spice/vnc password",
          .cmd        = hmp_set_password,
      },
SRST
-``set_password [ vnc | spice ] password [ action-if-connected ]``
-  Change spice/vnc password.  *action-if-connected* specifies what
-  should happen in case a connection is established: *fail* makes the
-  password change fail.  *disconnect* changes the password and
+``set_password [ vnc | spice ] password [ -d display ] [ action-if-connected 
]``
+  Change spice/vnc password.  *display* can be used with 'vnc' to specify
+  which display to set the password on.  *action-if-connected* specifies
+  what should happen in case a connection is established: *fail* makes
+  the password change fail.  *disconnect* changes the password and
    disconnects the client.  *keep* changes the password and keeps the
    connection up.  *keep* is the default.
  ERST
{
          .name       = "expire_password",
-        .args_type  = "protocol:s,time:s",
-        .params     = "protocol time",
+        .args_type  = "protocol:s,time:s,display:-dV",
+        .params     = "protocol time [-d display]",
          .help       = "set spice/vnc password expire-time",
          .cmd        = hmp_expire_password,
      },
SRST
-``expire_password [ vnc | spice ]`` *expire-time*
-  Specify when a password for spice/vnc becomes
-  invalid. *expire-time* accepts:
+``expire_password [ vnc | spice ] expire-time [ -d display ]``
+  Specify when a password for spice/vnc becomes invalid.
+  *display* behaves the same as in ``set_password``.
+  *expire-time* accepts:
``now``
      Invalidate password instantly.
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b8abe69609..daa4a8e106 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1451,26 +1451,39 @@ 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");
+    const char *display = qdict_get_try_str(qdict, "display");
      const char *connected = qdict_get_try_str(qdict, "connected");
      Error *err = NULL;
-    DisplayProtocol proto;
-    SetPasswordAction conn;
- proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                            DISPLAY_PROTOCOL_VNC, &err);
+    SetPasswordOptions opts = {
+        .password = g_strdup(password),
+        .u.vnc.display = NULL,
+    };
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
      if (err) {
          goto out;
      }
- conn = qapi_enum_parse(&SetPasswordAction_lookup, connected,
-                           SET_PASSWORD_ACTION_KEEP, &err);
-    if (err) {
-        goto out;
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    } else if (opts.protocol == DISPLAY_PROTOCOL_SPICE) {
+        opts.u.spice.has_connected = !!connected;
+        opts.u.spice.connected =
+            qapi_enum_parse(&SetPasswordAction_lookup, connected,
+                            SET_PASSWORD_ACTION_KEEP, &err);
+        if (err) {
+            goto out;
+        }
      }
- qmp_set_password(proto, password, !!connected, conn, &err);
+    qmp_set_password(&opts, &err);
out:
+    g_free(opts.password);
+    g_free(opts.u.vnc.display);

Uh-oh...

For DISPLAY_PROTOCOL_SPICE, we execute

            .u.vnc.display = NULL,
            opts.u.spice.has_connected = !!connected;
            opts.u.spice.connected =
                qapi_enum_parse(&SetPasswordAction_lookup, connected,
                                SET_PASSWORD_ACTION_KEEP, &err);
            opts.u.vnc.has_display = !!display;
        g_free(opts.u.vnc.display);

The assignments to opts.u.spice.has_connected and opts.u.spice_connected
clobber opts.u.vnc.display.

The simplest fix is to pass @display directly.  Precedence:
hmp_drive_mirror().

With "directly", I assume you mean without g_strdup, so:

  opts.u.vnc.display = (char *)display;

right? Does it matter that we drop the 'const'?


Do the same for @password, of course.

      hmp_handle_error(mon, err);
  }
@@ -1478,18 +1491,30 @@ 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");
+    const char *display = qdict_get_try_str(qdict, "display");
      Error *err = NULL;
-    DisplayProtocol proto;
- proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                            DISPLAY_PROTOCOL_VNC, &err);
+    ExpirePasswordOptions opts = {
+        .time = g_strdup(whenstr),
+        .u.vnc.display = NULL,
+    };
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
      if (err) {
          goto out;
      }
- qmp_expire_password(proto, whenstr, &err);
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = g_strdup(display);
+    }

Use of -d with spice are silently ignored.  Do we care?  Same for
hmp_set_password() above.

Depends on you, I don't. I think it's not worth catching even more
in HMP, since it's clear there's only one SPICE display anyway, and
it's all documented.


+
+    qmp_expire_password(&opts, &err);
out:
+    g_free(opts.time);
+    g_free(opts.u.vnc.display);

No uh-oh here, since opts.u.vnc is actually the only member of opts.u.
Still, let's pass @time and @display directly for consistency and
robustness.

      hmp_handle_error(mon, err);
  }

[...]






reply via email to

[Prev in Thread] Current Thread [Next in Thread]