[PATCH 0/9] ui: Move and clean up monitor command code

[PATCH 0/9] ui: Move and clean up monitor command code

[Markus Armbruster]

This is mainly about splitting off monitor-related code.  There's also
a few UI fixes to HMP commands sendkey and change vnc.

PATCH 3 drops long-disabled code.  We could bump the required version
of Spice instead.  Opinions?

Markus Armbruster (9):
  ui: Check numeric part of expire_password argument @time properly
  ui: Fix silent truncation of numeric keys in HMP sendkey
  ui: Drop disabled code for SPICE_CHANNEL_WEBDAV
  ui: Clean up a few things checkpatch.pl would flag later on
  ui: Move QMP commands from monitor to new ui/ui-qmp-cmds.c
  ui: Move HMP commands from monitor to new ui/ui-hmp-cmds.c
  ui: Improve "change vnc" error reporting
  ui: Factor out hmp_change_vnc(), and move to ui/ui-hmp-cmds.c
  ui: Reduce nesting in hmp_change_vnc() slightly

 include/monitor/hmp.h |   5 +
 monitor/hmp-cmds.c    | 370 +--------------------------------------
 monitor/qmp-cmds.c    | 163 ------------------
 ui/ui-hmp-cmds.c      | 390 ++++++++++++++++++++++++++++++++++++++++++
 ui/ui-qmp-cmds.c      | 194 +++++++++++++++++++++
 ui/meson.build        |   2 +
 6 files changed, 592 insertions(+), 532 deletions(-)
 create mode 100644 ui/ui-hmp-cmds.c
 create mode 100644 ui/ui-qmp-cmds.c

-- 
2.37.3

[PATCH 1/9] ui: Check numeric part of expire_password argument @time properly

[Markus Armbruster]

When argument @time isn't 'now' or 'never', we parse it as an integer,
optionally prefixed with '+'.  If parsing fails, we silently assume
zero.  Report an error and fail instead.

While there, use qemu_strtou64() instead of strtoull() so
checkpatch.pl won't complain.

Aside: encoding numbers in strings is bad QMP practice.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp-cmds.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 81c8fdadf8..054d7648b1 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -205,15 +205,28 @@ void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
     time_t when;
     int rc;
     const char *whenstr = opts->time;
+    const char *numstr = NULL;
+    uint64_t num;
 
     if (strcmp(whenstr, "now") == 0) {
         when = 0;
     } else if (strcmp(whenstr, "never") == 0) {
         when = TIME_MAX;
     } else if (whenstr[0] == '+') {
-        when = time(NULL) + strtoull(whenstr+1, NULL, 10);
+        when = time(NULL);
+        numstr = whenstr + 1;
     } else {
-        when = strtoull(whenstr, NULL, 10);
+        when = 0;
+        numstr = whenstr;
+    }
+
+    if (numstr) {
+        if (qemu_strtou64(numstr, NULL, 10, &num) < 0) {
+            error_setg(errp, "Parameter 'time' doesn't take value '%s'",
+                       whenstr);
+            return;
+        }
+        when += num;
     }
 
     if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
-- 
2.37.3

Re: [PATCH 1/9] ui: Check numeric part of expire_password argument @time properly

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:03AM +0100, Markus Armbruster wrote:
> When argument @time isn't 'now' or 'never', we parse it as an integer,
> optionally prefixed with '+'.  If parsing fails, we silently assume
> zero.  Report an error and fail instead.
> 
> While there, use qemu_strtou64() instead of strtoull() so
> checkpatch.pl won't complain.
> 
> Aside: encoding numbers in strings is bad QMP practice.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/qmp-cmds.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH 2/9] ui: Fix silent truncation of numeric keys in HMP sendkey

[Markus Armbruster]

Keys are int.  HMP sendkey assigns them from the value strtoul(),
silently truncating values greater than INT_MAX.  Fix to reject them.

While there, use qemu_strtoul() instead of strtoul() so checkpatch.pl
won't complain.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 01b789a79e..a7c9ae2520 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1666,8 +1666,13 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
         v = g_malloc0(sizeof(*v));
 
         if (strstart(keys, "0x", NULL)) {
-            char *endp;
-            int value = strtoul(keys, &endp, 0);
+            const char *endp;
+            unsigned long value;
+
+            if (qemu_strtoul(keys, &endp, 0, &value) < 0
+                || value >= INT_MAX) {
+                goto err_out;
+            }
             assert(endp <= keys + keyname_len);
             if (endp != keys + keyname_len) {
                 goto err_out;
-- 
2.37.3

Re: [PATCH 2/9] ui: Fix silent truncation of numeric keys in HMP sendkey

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:04AM +0100, Markus Armbruster wrote:
> Keys are int.  HMP sendkey assigns them from the value strtoul(),
> silently truncating values greater than INT_MAX.  Fix to reject them.
> 
> While there, use qemu_strtoul() instead of strtoul() so checkpatch.pl
> won't complain.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/hmp-cmds.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Markus Armbruster]

HMP "info spice" has a bit of code to show channel type
SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
"hmp: info spice: take out webdav" (v2.3.0), because it compiles only
with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.

Looks like nobody minded in more than seven years.  Drop it, so that
checkpatch.pl won't complain when I move the code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a7c9ae2520..86dd961462 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -626,12 +626,6 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
         [SPICE_CHANNEL_SMARTCARD] = "smartcard",
         [SPICE_CHANNEL_USBREDIR] = "usbredir",
         [SPICE_CHANNEL_PORT] = "port",
-#if 0
-        /* minimum spice-protocol is 0.12.3, webdav was added in 0.12.7,
-         * no easy way to #ifdef (SPICE_CHANNEL_* is a enum).  Disable
-         * as quick fix for build failures with older versions. */
-        [SPICE_CHANNEL_WEBDAV] = "webdav",
-#endif
     };
 
     info = qmp_query_spice(NULL);
-- 
2.37.3

Re: [PATCH 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
> HMP "info spice" has a bit of code to show channel type
> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.

Last version bump was 4 years ago

commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
Author: Marc-André Lureau <marcandre.lureau@redhat.com>
Date:   Wed Nov 28 19:59:32 2018 +0400

    configure: bump spice-server required version to 0.12.5

    ...snip....

    According to repology, all the distros that are build target platforms
    for QEMU include it:
    
          RHEL-7: 0.14.0
          Debian (Stretch): 0.12.8
          Debian (Jessie): 0.12.5
          FreeBSD (ports): 0.14.0
          OpenSUSE Leap 15: 0.14.0
          Ubuntu (Xenial): 0.12.6

We moved on from Debian and RHEL since then

   Debian 11: 0.14.3
   RHEL-8: 0.14.2
   FreeBSD (ports): 0.14.4
   Fedora 35: 0.14.0
   Ubuntu 20.04: 0.14.0
   OpenSUSE Leap 15.3: 0.14.3

IOW, we can bump to 0.14.0, and then revert the
webdav conditional commit.

> 
> Looks like nobody minded in more than seven years.  Drop it, so that
> checkpatch.pl won't complain when I move the code.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/hmp-cmds.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a7c9ae2520..86dd961462 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -626,12 +626,6 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict)
>          [SPICE_CHANNEL_SMARTCARD] = "smartcard",
>          [SPICE_CHANNEL_USBREDIR] = "usbredir",
>          [SPICE_CHANNEL_PORT] = "port",
> -#if 0
> -        /* minimum spice-protocol is 0.12.3, webdav was added in 0.12.7,
> -         * no easy way to #ifdef (SPICE_CHANNEL_* is a enum).  Disable
> -         * as quick fix for build failures with older versions. */
> -        [SPICE_CHANNEL_WEBDAV] = "webdav",
> -#endif
>      };
>  
>      info = qmp_query_spice(NULL);
> -- 
> 2.37.3
> 
> 

With 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 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
>> HMP "info spice" has a bit of code to show channel type
>> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
>> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
>> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.
>
> Last version bump was 4 years ago
>
> commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Wed Nov 28 19:59:32 2018 +0400
>
>     configure: bump spice-server required version to 0.12.5
>
>     ...snip....
>
>     According to repology, all the distros that are build target platforms
>     for QEMU include it:
>     
>           RHEL-7: 0.14.0
>           Debian (Stretch): 0.12.8
>           Debian (Jessie): 0.12.5
>           FreeBSD (ports): 0.14.0
>           OpenSUSE Leap 15: 0.14.0
>           Ubuntu (Xenial): 0.12.6
>
> We moved on from Debian and RHEL since then
>
>    Debian 11: 0.14.3
>    RHEL-8: 0.14.2
>    FreeBSD (ports): 0.14.4
>    Fedora 35: 0.14.0
>    Ubuntu 20.04: 0.14.0
>    OpenSUSE Leap 15.3: 0.14.3
>
> IOW, we can bump to 0.14.0, and then revert the
> webdav conditional commit.

Will do in v2.  Thanks!

Re: [PATCH 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
>> HMP "info spice" has a bit of code to show channel type
>> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
>> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
>> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.
>
> Last version bump was 4 years ago
>
> commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
> Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> Date:   Wed Nov 28 19:59:32 2018 +0400
>
>     configure: bump spice-server required version to 0.12.5
>
>     ...snip....
>
>     According to repology, all the distros that are build target platforms
>     for QEMU include it:
>     
>           RHEL-7: 0.14.0
>           Debian (Stretch): 0.12.8
>           Debian (Jessie): 0.12.5
>           FreeBSD (ports): 0.14.0
>           OpenSUSE Leap 15: 0.14.0
>           Ubuntu (Xenial): 0.12.6
>
> We moved on from Debian and RHEL since then
>
>    Debian 11: 0.14.3
>    RHEL-8: 0.14.2
>    FreeBSD (ports): 0.14.4
>    Fedora 35: 0.14.0
>    Ubuntu 20.04: 0.14.0
>    OpenSUSE Leap 15.3: 0.14.3
>
> IOW, we can bump to 0.14.0, and then revert the
> webdav conditional commit.

We need to bump spice-protocol, actually.

Would you like me to bump spice-server as well?  To which version?

[...]

Re: [PATCH 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 01:39:13PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
> >> HMP "info spice" has a bit of code to show channel type
> >> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
> >> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
> >> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.
> >
> > Last version bump was 4 years ago
> >
> > commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
> > Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Date:   Wed Nov 28 19:59:32 2018 +0400
> >
> >     configure: bump spice-server required version to 0.12.5
> >
> >     ...snip....
> >
> >     According to repology, all the distros that are build target platforms
> >     for QEMU include it:
> >     
> >           RHEL-7: 0.14.0
> >           Debian (Stretch): 0.12.8
> >           Debian (Jessie): 0.12.5
> >           FreeBSD (ports): 0.14.0
> >           OpenSUSE Leap 15: 0.14.0
> >           Ubuntu (Xenial): 0.12.6
> >
> > We moved on from Debian and RHEL since then
> >
> >    Debian 11: 0.14.3
> >    RHEL-8: 0.14.2
> >    FreeBSD (ports): 0.14.4
> >    Fedora 35: 0.14.0
> >    Ubuntu 20.04: 0.14.0
> >    OpenSUSE Leap 15.3: 0.14.3
> >
> > IOW, we can bump to 0.14.0, and then revert the
> > webdav conditional commit.
> 
> We need to bump spice-protocol, actually.

Opps, I'm getting mixed up. The commit I mentioned was spice-server,
but the new versions I've listed just were indeed spice-protocol

> Would you like me to bump spice-server as well?  To which version?

Yes, might as well, the spice-server versions are slightly different:

     Debian 11: 0.14.3
     RHEL-8: 0.14.3
     FreeBSD (ports): 0.15.0
     Fedora 35: 0.15.0
     Ubuntu 20.04: 0.14.2
     OpenSUSE Leap 15.3: 0.14.3
 
I think we might as well pick  0.14.0 for both protocol and server.

With 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 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 01, 2022 at 01:39:13PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
>> >> HMP "info spice" has a bit of code to show channel type
>> >> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
>> >> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
>> >> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.
>> >
>> > Last version bump was 4 years ago
>> >
>> > commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
>> > Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Date:   Wed Nov 28 19:59:32 2018 +0400
>> >
>> >     configure: bump spice-server required version to 0.12.5
>> >
>> >     ...snip....
>> >
>> >     According to repology, all the distros that are build target platforms
>> >     for QEMU include it:
>> >     
>> >           RHEL-7: 0.14.0
>> >           Debian (Stretch): 0.12.8
>> >           Debian (Jessie): 0.12.5
>> >           FreeBSD (ports): 0.14.0
>> >           OpenSUSE Leap 15: 0.14.0
>> >           Ubuntu (Xenial): 0.12.6
>> >
>> > We moved on from Debian and RHEL since then
>> >
>> >    Debian 11: 0.14.3
>> >    RHEL-8: 0.14.2
>> >    FreeBSD (ports): 0.14.4
>> >    Fedora 35: 0.14.0
>> >    Ubuntu 20.04: 0.14.0
>> >    OpenSUSE Leap 15.3: 0.14.3
>> >
>> > IOW, we can bump to 0.14.0, and then revert the
>> > webdav conditional commit.
>> 
>> We need to bump spice-protocol, actually.
>
> Opps, I'm getting mixed up. The commit I mentioned was spice-server,
> but the new versions I've listed just were indeed spice-protocol
>
>> Would you like me to bump spice-server as well?  To which version?
>
> Yes, might as well, the spice-server versions are slightly different:
>
>      Debian 11: 0.14.3
>      RHEL-8: 0.14.3
>      FreeBSD (ports): 0.15.0
>      Fedora 35: 0.15.0
>      Ubuntu 20.04: 0.14.2
>      OpenSUSE Leap 15.3: 0.14.3
>  
> I think we might as well pick  0.14.0 for both protocol and server.

Makes sense, but it leads to another question.

I found obvious version checks for spice-protocol, and dropped the
outmoded ones, namely

    #if CHECK_SPICE_PROTOCOL_VERSION(0, 14, 0)

For spice-server, I see a bunch of SPICE_INTERFACE_FOO_{MAJOR,MINOR} we
check, and which ones become outmoded is not obvious to me.  Help?

Re: [PATCH 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 04:49:25PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Dec 01, 2022 at 01:39:13PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Thu, Dec 01, 2022 at 07:13:05AM +0100, Markus Armbruster wrote:
> >> >> HMP "info spice" has a bit of code to show channel type
> >> >> SPICE_CHANNEL_WEBDAV as "webdav", disabled since commit 7c6044a94e
> >> >> "hmp: info spice: take out webdav" (v2.3.0), because it compiles only
> >> >> with Spice versions 0.12.7 and later.  Our minimum version is 0.12.5.
> >> >
> >> > Last version bump was 4 years ago
> >> >
> >> > commit 1b63665c2c0e0d52735e0dd5217f109fe0dd2322
> >> > Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > Date:   Wed Nov 28 19:59:32 2018 +0400
> >> >
> >> >     configure: bump spice-server required version to 0.12.5
> >> >
> >> >     ...snip....
> >> >
> >> >     According to repology, all the distros that are build target platforms
> >> >     for QEMU include it:
> >> >     
> >> >           RHEL-7: 0.14.0
> >> >           Debian (Stretch): 0.12.8
> >> >           Debian (Jessie): 0.12.5
> >> >           FreeBSD (ports): 0.14.0
> >> >           OpenSUSE Leap 15: 0.14.0
> >> >           Ubuntu (Xenial): 0.12.6
> >> >
> >> > We moved on from Debian and RHEL since then
> >> >
> >> >    Debian 11: 0.14.3
> >> >    RHEL-8: 0.14.2
> >> >    FreeBSD (ports): 0.14.4
> >> >    Fedora 35: 0.14.0
> >> >    Ubuntu 20.04: 0.14.0
> >> >    OpenSUSE Leap 15.3: 0.14.3
> >> >
> >> > IOW, we can bump to 0.14.0, and then revert the
> >> > webdav conditional commit.
> >> 
> >> We need to bump spice-protocol, actually.
> >
> > Opps, I'm getting mixed up. The commit I mentioned was spice-server,
> > but the new versions I've listed just were indeed spice-protocol
> >
> >> Would you like me to bump spice-server as well?  To which version?
> >
> > Yes, might as well, the spice-server versions are slightly different:
> >
> >      Debian 11: 0.14.3
> >      RHEL-8: 0.14.3
> >      FreeBSD (ports): 0.15.0
> >      Fedora 35: 0.15.0
> >      Ubuntu 20.04: 0.14.2
> >      OpenSUSE Leap 15.3: 0.14.3
> >  
> > I think we might as well pick  0.14.0 for both protocol and server.
> 
> Makes sense, but it leads to another question.
> 
> I found obvious version checks for spice-protocol, and dropped the
> outmoded ones, namely
> 
>     #if CHECK_SPICE_PROTOCOL_VERSION(0, 14, 0)
> 
> For spice-server, I see a bunch of SPICE_INTERFACE_FOO_{MAJOR,MINOR} we
> check, and which ones become outmoded is not obvious to me.  Help?

Ignore all the interface ones. For the server, the check to look
for is against SPICE_SERVER_VERSION

chardev/spice.c:#if SPICE_SERVER_VERSION >= 0x000c06
chardev/spice.c:#if SPICE_SERVER_VERSION < 0x000e02
hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 && SPICE_SERVER_VERSION < 0x000e02
hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
hw/display/qxl.h:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
include/ui/qemu-spice.h:#if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
include/ui/qemu-spice.h:#if defined(SPICE_SERVER_VERSION) && (SPICE_SERVER_VERSION >= 0x000f00)
include/ui/spice-display.h:# if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */
ui/spice-display.c:#if SPICE_SERVER_VERSION >= 0x000e04 /* release 0.14.4 */
ui/spice-display.c:#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */

A fair few of those will be obsolete

With 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 3/9] ui: Drop disabled code for SPICE_CHANNEL_WEBDAV

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 01, 2022 at 04:49:25PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Dec 01, 2022 at 01:39:13PM +0100, Markus Armbruster wrote:

[...]

>> >> Would you like me to bump spice-server as well?  To which version?
>> >
>> > Yes, might as well, the spice-server versions are slightly different:
>> >
>> >      Debian 11: 0.14.3
>> >      RHEL-8: 0.14.3
>> >      FreeBSD (ports): 0.15.0
>> >      Fedora 35: 0.15.0
>> >      Ubuntu 20.04: 0.14.2
>> >      OpenSUSE Leap 15.3: 0.14.3
>> >  
>> > I think we might as well pick  0.14.0 for both protocol and server.
>> 
>> Makes sense, but it leads to another question.
>> 
>> I found obvious version checks for spice-protocol, and dropped the
>> outmoded ones, namely
>> 
>>     #if CHECK_SPICE_PROTOCOL_VERSION(0, 14, 0)
>> 
>> For spice-server, I see a bunch of SPICE_INTERFACE_FOO_{MAJOR,MINOR} we
>> check, and which ones become outmoded is not obvious to me.  Help?
>
> Ignore all the interface ones. For the server, the check to look
> for is against SPICE_SERVER_VERSION
>
> chardev/spice.c:#if SPICE_SERVER_VERSION >= 0x000c06
> chardev/spice.c:#if SPICE_SERVER_VERSION < 0x000e02
> hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 && SPICE_SERVER_VERSION < 0x000e02
> hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
> hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
> hw/display/qxl.c:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
> hw/display/qxl.h:#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
> include/ui/qemu-spice.h:#if !defined(SPICE_SERVER_VERSION) || (SPICE_SERVER_VERSION < 0xc06)
> include/ui/qemu-spice.h:#if defined(SPICE_SERVER_VERSION) && (SPICE_SERVER_VERSION >= 0x000f00)
> include/ui/spice-display.h:# if SPICE_SERVER_VERSION >= 0x000d01 /* release 0.13.1 */
> ui/spice-display.c:#if SPICE_SERVER_VERSION >= 0x000e04 /* release 0.14.4 */
> ui/spice-display.c:#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
>
> A fair few of those will be obsolete

Got it, thanks!

[PATCH 4/9] ui: Clean up a few things checkpatch.pl would flag later on

[Markus Armbruster]

Fix a few style violations so that checkpatch.pl won't complain when I
move this code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c |  7 ++++---
 monitor/qmp-cmds.c | 21 +++++++++++----------
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 86dd961462..29fcc730cb 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -591,9 +591,10 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
         hmp_info_vnc_servers(mon, info->server);
         hmp_info_vnc_clients(mon, info->clients);
         if (!info->server) {
-            /* The server entry displays its auth, we only
-             * need to display in the case of 'reverse' connections
-             * where there's no server.
+            /*
+             * The server entry displays its auth, we only need to
+             * display in the case of 'reverse' connections where
+             * there's no server.
              */
             hmp_info_vnc_authcrypt(mon, "  ", info->auth,
                                info->has_vencrypt ? &info->vencrypt : NULL);
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 054d7648b1..a7c95e8e39 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -190,8 +190,10 @@ void qmp_set_password(SetPasswordOptions *opts, Error **errp)
             error_setg(errp, QERR_INVALID_PARAMETER, "connected");
             return;
         }
-        /* Note that setting an empty password will not disable login through
-         * this interface. */
+        /*
+         * Note that setting an empty password will not disable login
+         * through this interface.
+         */
         rc = vnc_display_password(opts->u.vnc.display, opts->password);
     }
 
@@ -276,12 +278,10 @@ void qmp_add_client(const char *protocol, const char *fdname,
             error_setg(errp, "spice failed to add client");
             close(fd);
         }
-        return;
 #ifdef CONFIG_VNC
     } else if (strcmp(protocol, "vnc") == 0) {
         skipauth = has_skipauth ? skipauth : false;
         vnc_display_add_client(NULL, fd, skipauth);
-        return;
 #endif
 #ifdef CONFIG_DBUS_DISPLAY
     } else if (strcmp(protocol, "@dbus-display") == 0) {
@@ -293,19 +293,20 @@ void qmp_add_client(const char *protocol, const char *fdname,
             close(fd);
             return;
         }
-        return;
 #endif
-    } else if ((s = qemu_chr_find(protocol)) != NULL) {
+    } else {
+        s = qemu_chr_find(protocol);
+        if (!s) {
+            error_setg(errp, "protocol '%s' is invalid", protocol);
+            close(fd);
+            return;
+        }
         if (qemu_chr_add_client(s, fd) < 0) {
             error_setg(errp, "failed to add client");
             close(fd);
             return;
         }
-        return;
     }
-
-    error_setg(errp, "protocol '%s' is invalid", protocol);
-    close(fd);
 }
 
 
-- 
2.37.3

Re: [PATCH 4/9] ui: Clean up a few things checkpatch.pl would flag later on

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:06AM +0100, Markus Armbruster wrote:
> Fix a few style violations so that checkpatch.pl won't complain when I
> move this code.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/hmp-cmds.c |  7 ++++---
>  monitor/qmp-cmds.c | 21 +++++++++++----------
>  2 files changed, 15 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH 5/9] ui: Move QMP commands from monitor to new ui/ui-qmp-cmds.c

[Markus Armbruster]

This moves these commands from MAINTAINERS section "Human
Monitor (HMP)" to "Graphics".

Command add-client applies to socket character devices in addition to
display devices.  Move it anyway.  Aside: the way @protocol character
device IDs and display types is bad design.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/qmp-cmds.c | 177 -----------------------------------------
 ui/ui-qmp-cmds.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++
 ui/meson.build     |   1 +
 3 files changed, 195 insertions(+), 177 deletions(-)
 create mode 100644 ui/ui-qmp-cmds.c

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index a7c95e8e39..85b2089873 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -16,14 +16,9 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qemu/option.h"
-#include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
 #include "qemu/config-file.h"
 #include "qemu/uuid.h"
-#include "chardev/char.h"
-#include "ui/qemu-spice.h"
-#include "ui/console.h"
-#include "ui/dbus-display.h"
 #include "sysemu/kvm.h"
 #include "sysemu/runstate.h"
 #include "sysemu/runstate-action.h"
@@ -36,9 +31,7 @@
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-commands-stats.h"
-#include "qapi/qapi-commands-ui.h"
 #include "qapi/type-helpers.h"
-#include "qapi/qmp/qerror.h"
 #include "exec/ramlist.h"
 #include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
@@ -172,144 +165,6 @@ void qmp_system_wakeup(Error **errp)
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, errp);
 }
 
-void qmp_set_password(SetPasswordOptions *opts, Error **errp)
-{
-    int rc;
-
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
-        if (!qemu_using_spice(errp)) {
-            return;
-        }
-        rc = qemu_spice.set_passwd(opts->password,
-                opts->connected == SET_PASSWORD_ACTION_FAIL,
-                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
-    } else {
-        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
-        if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
-            /* vnc supports "connected=keep" only */
-            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
-            return;
-        }
-        /*
-         * Note that setting an empty password will not disable login
-         * through this interface.
-         */
-        rc = vnc_display_password(opts->u.vnc.display, opts->password);
-    }
-
-    if (rc != 0) {
-        error_setg(errp, "Could not set password");
-    }
-}
-
-void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
-{
-    time_t when;
-    int rc;
-    const char *whenstr = opts->time;
-    const char *numstr = NULL;
-    uint64_t num;
-
-    if (strcmp(whenstr, "now") == 0) {
-        when = 0;
-    } else if (strcmp(whenstr, "never") == 0) {
-        when = TIME_MAX;
-    } else if (whenstr[0] == '+') {
-        when = time(NULL);
-        numstr = whenstr + 1;
-    } else {
-        when = 0;
-        numstr = whenstr;
-    }
-
-    if (numstr) {
-        if (qemu_strtou64(numstr, NULL, 10, &num) < 0) {
-            error_setg(errp, "Parameter 'time' doesn't take value '%s'",
-                       whenstr);
-            return;
-        }
-        when += num;
-    }
-
-    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
-        if (!qemu_using_spice(errp)) {
-            return;
-        }
-        rc = qemu_spice.set_pw_expire(when);
-    } else {
-        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
-        rc = vnc_display_pw_expire(opts->u.vnc.display, when);
-    }
-
-    if (rc != 0) {
-        error_setg(errp, "Could not set password expire time");
-    }
-}
-
-#ifdef CONFIG_VNC
-void qmp_change_vnc_password(const char *password, Error **errp)
-{
-    if (vnc_display_password(NULL, password) < 0) {
-        error_setg(errp, "Could not set password");
-    }
-}
-#endif
-
-void qmp_add_client(const char *protocol, const char *fdname,
-                    bool has_skipauth, bool skipauth, bool has_tls, bool tls,
-                    Error **errp)
-{
-    Chardev *s;
-    int fd;
-
-    fd = monitor_get_fd(monitor_cur(), fdname, errp);
-    if (fd < 0) {
-        return;
-    }
-
-    if (strcmp(protocol, "spice") == 0) {
-        if (!qemu_using_spice(errp)) {
-            close(fd);
-            return;
-        }
-        skipauth = has_skipauth ? skipauth : false;
-        tls = has_tls ? tls : false;
-        if (qemu_spice.display_add_client(fd, skipauth, tls) < 0) {
-            error_setg(errp, "spice failed to add client");
-            close(fd);
-        }
-#ifdef CONFIG_VNC
-    } else if (strcmp(protocol, "vnc") == 0) {
-        skipauth = has_skipauth ? skipauth : false;
-        vnc_display_add_client(NULL, fd, skipauth);
-#endif
-#ifdef CONFIG_DBUS_DISPLAY
-    } else if (strcmp(protocol, "@dbus-display") == 0) {
-        if (!qemu_using_dbus_display(errp)) {
-            close(fd);
-            return;
-        }
-        if (!qemu_dbus_display.add_client(fd, errp)) {
-            close(fd);
-            return;
-        }
-#endif
-    } else {
-        s = qemu_chr_find(protocol);
-        if (!s) {
-            error_setg(errp, "protocol '%s' is invalid", protocol);
-            close(fd);
-            return;
-        }
-        if (qemu_chr_add_client(s, fd) < 0) {
-            error_setg(errp, "failed to add client");
-            close(fd);
-            return;
-        }
-    }
-}
-
-
 MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
 {
     return qmp_memory_device_list();
@@ -348,38 +203,6 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
     return mem_info;
 }
 
-void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
-{
-    switch (arg->type) {
-    case DISPLAY_RELOAD_TYPE_VNC:
-#ifdef CONFIG_VNC
-        if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) {
-            vnc_display_reload_certs(NULL, errp);
-        }
-#else
-        error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
-#endif
-        break;
-    default:
-        abort();
-    }
-}
-
-void qmp_display_update(DisplayUpdateOptions *arg, Error **errp)
-{
-    switch (arg->type) {
-    case DISPLAY_UPDATE_TYPE_VNC:
-#ifdef CONFIG_VNC
-        vnc_display_update(&arg->u.vnc, errp);
-#else
-        error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
-#endif
-        break;
-    default:
-        abort();
-    }
-}
-
 static int qmp_x_query_rdma_foreach(Object *obj, void *opaque)
 {
     RdmaProvider *rdma;
diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
new file mode 100644
index 0000000000..5d145c26f7
--- /dev/null
+++ b/ui/ui-qmp-cmds.c
@@ -0,0 +1,194 @@
+/*
+ * QMP commands related to UI
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "chardev/char.h"
+#include "monitor/monitor.h"
+#include "qapi/qapi-commands-misc.h"
+#include "qapi/qapi-commands-ui.h"
+#include "qapi/qmp/qerror.h"
+#include "qemu/cutils.h"
+#include "ui/console.h"
+#include "ui/dbus-display.h"
+#include "ui/qemu-spice.h"
+
+void qmp_set_password(SetPasswordOptions *opts, Error **errp)
+{
+    int rc;
+
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+        if (!qemu_using_spice(errp)) {
+            return;
+        }
+        rc = qemu_spice.set_passwd(opts->password,
+                opts->connected == SET_PASSWORD_ACTION_FAIL,
+                opts->connected == SET_PASSWORD_ACTION_DISCONNECT);
+    } else {
+        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        if (opts->connected != SET_PASSWORD_ACTION_KEEP) {
+            /* vnc supports "connected=keep" only */
+            error_setg(errp, QERR_INVALID_PARAMETER, "connected");
+            return;
+        }
+        /*
+         * Note that setting an empty password will not disable login
+         * through this interface.
+         */
+        rc = vnc_display_password(opts->u.vnc.display, opts->password);
+    }
+
+    if (rc != 0) {
+        error_setg(errp, "Could not set password");
+    }
+}
+
+void qmp_expire_password(ExpirePasswordOptions *opts, Error **errp)
+{
+    time_t when;
+    int rc;
+    const char *whenstr = opts->time;
+    const char *numstr = NULL;
+    uint64_t num;
+
+    if (strcmp(whenstr, "now") == 0) {
+        when = 0;
+    } else if (strcmp(whenstr, "never") == 0) {
+        when = TIME_MAX;
+    } else if (whenstr[0] == '+') {
+        when = time(NULL);
+        numstr = whenstr + 1;
+    } else {
+        when = 0;
+        numstr = whenstr;
+    }
+
+    if (numstr) {
+        if (qemu_strtou64(numstr, NULL, 10, &num) < 0) {
+            error_setg(errp, "Parameter 'time' doesn't take value '%s'",
+                       whenstr);
+            return;
+        }
+        when += num;
+    }
+
+    if (opts->protocol == DISPLAY_PROTOCOL_SPICE) {
+        if (!qemu_using_spice(errp)) {
+            return;
+        }
+        rc = qemu_spice.set_pw_expire(when);
+    } else {
+        assert(opts->protocol == DISPLAY_PROTOCOL_VNC);
+        rc = vnc_display_pw_expire(opts->u.vnc.display, when);
+    }
+
+    if (rc != 0) {
+        error_setg(errp, "Could not set password expire time");
+    }
+}
+
+#ifdef CONFIG_VNC
+void qmp_change_vnc_password(const char *password, Error **errp)
+{
+    if (vnc_display_password(NULL, password) < 0) {
+        error_setg(errp, "Could not set password");
+    }
+}
+#endif
+
+void qmp_add_client(const char *protocol, const char *fdname,
+                    bool has_skipauth, bool skipauth, bool has_tls, bool tls,
+                    Error **errp)
+{
+    Chardev *s;
+    int fd;
+
+    fd = monitor_get_fd(monitor_cur(), fdname, errp);
+    if (fd < 0) {
+        return;
+    }
+
+    if (strcmp(protocol, "spice") == 0) {
+        if (!qemu_using_spice(errp)) {
+            close(fd);
+            return;
+        }
+        skipauth = has_skipauth ? skipauth : false;
+        tls = has_tls ? tls : false;
+        if (qemu_spice.display_add_client(fd, skipauth, tls) < 0) {
+            error_setg(errp, "spice failed to add client");
+            close(fd);
+        }
+#ifdef CONFIG_VNC
+    } else if (strcmp(protocol, "vnc") == 0) {
+        skipauth = has_skipauth ? skipauth : false;
+        vnc_display_add_client(NULL, fd, skipauth);
+#endif
+#ifdef CONFIG_DBUS_DISPLAY
+    } else if (strcmp(protocol, "@dbus-display") == 0) {
+        if (!qemu_using_dbus_display(errp)) {
+            close(fd);
+            return;
+        }
+        if (!qemu_dbus_display.add_client(fd, errp)) {
+            close(fd);
+            return;
+        }
+#endif
+    } else {
+        s = qemu_chr_find(protocol);
+        if (!s) {
+            error_setg(errp, "protocol '%s' is invalid", protocol);
+            close(fd);
+            return;
+        }
+        if (qemu_chr_add_client(s, fd) < 0) {
+            error_setg(errp, "failed to add client");
+            close(fd);
+            return;
+        }
+    }
+}
+
+void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
+{
+    switch (arg->type) {
+    case DISPLAY_RELOAD_TYPE_VNC:
+#ifdef CONFIG_VNC
+        if (arg->u.vnc.has_tls_certs && arg->u.vnc.tls_certs) {
+            vnc_display_reload_certs(NULL, errp);
+        }
+#else
+        error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
+#endif
+        break;
+    default:
+        abort();
+    }
+}
+
+void qmp_display_update(DisplayUpdateOptions *arg, Error **errp)
+{
+    switch (arg->type) {
+    case DISPLAY_UPDATE_TYPE_VNC:
+#ifdef CONFIG_VNC
+        vnc_display_update(&arg->u.vnc, errp);
+#else
+        error_setg(errp, "vnc is invalid, missing 'CONFIG_VNC'");
+#endif
+        break;
+    default:
+        abort();
+    }
+}
diff --git a/ui/meson.build b/ui/meson.build
index c1b137bf33..9194ea335b 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -14,6 +14,7 @@ softmmu_ss.add(files(
   'kbd-state.c',
   'keymaps.c',
   'qemu-pixman.c',
+  'ui-qmp-cmds.c',
   'util.c',
 ))
 if dbus_display
-- 
2.37.3

Re: [PATCH 5/9] ui: Move QMP commands from monitor to new ui/ui-qmp-cmds.c

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> This moves these commands from MAINTAINERS section "Human
> Monitor (HMP)" to "Graphics".
>
> Command add-client applies to socket character devices in addition to
> display devices.  Move it anyway.  Aside: the way @protocol character
> device IDs and display types is bad design.

Last sentence no verb.  Trying again:

  Aside: @protocol is either a display type or else a character device
  ID.  Bad design.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 5/9] ui: Move QMP commands from monitor to new ui/ui-qmp-cmds.c

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> This moves these commands from MAINTAINERS section "Human
>> Monitor (HMP)" to "Graphics".

Make that 'section "QMP"', of course.

>> Command add-client applies to socket character devices in addition to
>> display devices.  Move it anyway.  Aside: the way @protocol character
>> device IDs and display types is bad design.
>
> Last sentence no verb.  Trying again:
>
>   Aside: @protocol is either a display type or else a character device
>   ID.  Bad design.
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 5/9] ui: Move QMP commands from monitor to new ui/ui-qmp-cmds.c

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:07AM +0100, Markus Armbruster wrote:
> This moves these commands from MAINTAINERS section "Human
> Monitor (HMP)" to "Graphics".
> 
> Command add-client applies to socket character devices in addition to
> display devices.  Move it anyway.  Aside: the way @protocol character
> device IDs and display types is bad design.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/qmp-cmds.c | 177 -----------------------------------------
>  ui/ui-qmp-cmds.c   | 194 +++++++++++++++++++++++++++++++++++++++++++++
>  ui/meson.build     |   1 +
>  3 files changed, 195 insertions(+), 177 deletions(-)
>  create mode 100644 ui/ui-qmp-cmds.c

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH 6/9] ui: Move HMP commands from monitor to new ui/ui-hmp-cmds.c

[Markus Armbruster]

This moves these commands from MAINTAINERS section "Human
Monitor (HMP)" to "Graphics".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c | 342 ------------------------------------------
 ui/ui-hmp-cmds.c   | 360 +++++++++++++++++++++++++++++++++++++++++++++
 ui/meson.build     |   1 +
 3 files changed, 361 insertions(+), 342 deletions(-)
 create mode 100644 ui/ui-hmp-cmds.c

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 29fcc730cb..f0f7b74fb3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -52,7 +52,6 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qom/object_interfaces.h"
-#include "ui/console.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
 #include "hw/core/cpu.h"
@@ -60,10 +59,6 @@
 #include "migration/snapshot.h"
 #include "migration/misc.h"
 
-#ifdef CONFIG_SPICE
-#include <spice/enums.h>
-#endif
-
 bool hmp_handle_error(Monitor *mon, Error *err)
 {
     if (err) {
@@ -179,26 +174,6 @@ void hmp_info_chardev(Monitor *mon, const QDict *qdict)
     qapi_free_ChardevInfoList(char_info);
 }
 
-void hmp_info_mice(Monitor *mon, const QDict *qdict)
-{
-    MouseInfoList *mice_list, *mouse;
-
-    mice_list = qmp_query_mice(NULL);
-    if (!mice_list) {
-        monitor_printf(mon, "No mouse devices connected\n");
-        return;
-    }
-
-    for (mouse = mice_list; mouse; mouse = mouse->next) {
-        monitor_printf(mon, "%c Mouse #%" PRId64 ": %s%s\n",
-                       mouse->value->current ? '*' : ' ',
-                       mouse->value->index, mouse->value->name,
-                       mouse->value->absolute ? " (absolute)" : "");
-    }
-
-    qapi_free_MouseInfoList(mice_list);
-}
-
 void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 {
     MigrationInfo *info;
@@ -518,169 +493,6 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
     qapi_free_MigrationParameters(params);
 }
 
-
-#ifdef CONFIG_VNC
-/* Helper for hmp_info_vnc_clients, _servers */
-static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
-                                  const char *name)
-{
-    monitor_printf(mon, "  %s: %s:%s (%s%s)\n",
-                   name,
-                   info->host,
-                   info->service,
-                   NetworkAddressFamily_str(info->family),
-                   info->websocket ? " (Websocket)" : "");
-}
-
-/* Helper displaying and auth and crypt info */
-static void hmp_info_vnc_authcrypt(Monitor *mon, const char *indent,
-                                   VncPrimaryAuth auth,
-                                   VncVencryptSubAuth *vencrypt)
-{
-    monitor_printf(mon, "%sAuth: %s (Sub: %s)\n", indent,
-                   VncPrimaryAuth_str(auth),
-                   vencrypt ? VncVencryptSubAuth_str(*vencrypt) : "none");
-}
-
-static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
-{
-    while (client) {
-        VncClientInfo *cinfo = client->value;
-
-        hmp_info_VncBasicInfo(mon, qapi_VncClientInfo_base(cinfo), "Client");
-        monitor_printf(mon, "    x509_dname: %s\n",
-                       cinfo->has_x509_dname ?
-                       cinfo->x509_dname : "none");
-        monitor_printf(mon, "    sasl_username: %s\n",
-                       cinfo->has_sasl_username ?
-                       cinfo->sasl_username : "none");
-
-        client = client->next;
-    }
-}
-
-static void hmp_info_vnc_servers(Monitor *mon, VncServerInfo2List *server)
-{
-    while (server) {
-        VncServerInfo2 *sinfo = server->value;
-        hmp_info_VncBasicInfo(mon, qapi_VncServerInfo2_base(sinfo), "Server");
-        hmp_info_vnc_authcrypt(mon, "    ", sinfo->auth,
-                               sinfo->has_vencrypt ? &sinfo->vencrypt : NULL);
-        server = server->next;
-    }
-}
-
-void hmp_info_vnc(Monitor *mon, const QDict *qdict)
-{
-    VncInfo2List *info2l, *info2l_head;
-    Error *err = NULL;
-
-    info2l = qmp_query_vnc_servers(&err);
-    info2l_head = info2l;
-    if (hmp_handle_error(mon, err)) {
-        return;
-    }
-    if (!info2l) {
-        monitor_printf(mon, "None\n");
-        return;
-    }
-
-    while (info2l) {
-        VncInfo2 *info = info2l->value;
-        monitor_printf(mon, "%s:\n", info->id);
-        hmp_info_vnc_servers(mon, info->server);
-        hmp_info_vnc_clients(mon, info->clients);
-        if (!info->server) {
-            /*
-             * The server entry displays its auth, we only need to
-             * display in the case of 'reverse' connections where
-             * there's no server.
-             */
-            hmp_info_vnc_authcrypt(mon, "  ", info->auth,
-                               info->has_vencrypt ? &info->vencrypt : NULL);
-        }
-        if (info->has_display) {
-            monitor_printf(mon, "  Display: %s\n", info->display);
-        }
-        info2l = info2l->next;
-    }
-
-    qapi_free_VncInfo2List(info2l_head);
-
-}
-#endif
-
-#ifdef CONFIG_SPICE
-void hmp_info_spice(Monitor *mon, const QDict *qdict)
-{
-    SpiceChannelList *chan;
-    SpiceInfo *info;
-    const char *channel_name;
-    const char * const channel_names[] = {
-        [SPICE_CHANNEL_MAIN] = "main",
-        [SPICE_CHANNEL_DISPLAY] = "display",
-        [SPICE_CHANNEL_INPUTS] = "inputs",
-        [SPICE_CHANNEL_CURSOR] = "cursor",
-        [SPICE_CHANNEL_PLAYBACK] = "playback",
-        [SPICE_CHANNEL_RECORD] = "record",
-        [SPICE_CHANNEL_TUNNEL] = "tunnel",
-        [SPICE_CHANNEL_SMARTCARD] = "smartcard",
-        [SPICE_CHANNEL_USBREDIR] = "usbredir",
-        [SPICE_CHANNEL_PORT] = "port",
-    };
-
-    info = qmp_query_spice(NULL);
-
-    if (!info->enabled) {
-        monitor_printf(mon, "Server: disabled\n");
-        goto out;
-    }
-
-    monitor_printf(mon, "Server:\n");
-    if (info->has_port) {
-        monitor_printf(mon, "     address: %s:%" PRId64 "\n",
-                       info->host, info->port);
-    }
-    if (info->has_tls_port) {
-        monitor_printf(mon, "     address: %s:%" PRId64 " [tls]\n",
-                       info->host, info->tls_port);
-    }
-    monitor_printf(mon, "    migrated: %s\n",
-                   info->migrated ? "true" : "false");
-    monitor_printf(mon, "        auth: %s\n", info->auth);
-    monitor_printf(mon, "    compiled: %s\n", info->compiled_version);
-    monitor_printf(mon, "  mouse-mode: %s\n",
-                   SpiceQueryMouseMode_str(info->mouse_mode));
-
-    if (!info->has_channels || info->channels == NULL) {
-        monitor_printf(mon, "Channels: none\n");
-    } else {
-        for (chan = info->channels; chan; chan = chan->next) {
-            monitor_printf(mon, "Channel:\n");
-            monitor_printf(mon, "     address: %s:%s%s\n",
-                           chan->value->host, chan->value->port,
-                           chan->value->tls ? " [tls]" : "");
-            monitor_printf(mon, "     session: %" PRId64 "\n",
-                           chan->value->connection_id);
-            monitor_printf(mon, "     channel: %" PRId64 ":%" PRId64 "\n",
-                           chan->value->channel_type, chan->value->channel_id);
-
-            channel_name = "unknown";
-            if (chan->value->channel_type > 0 &&
-                chan->value->channel_type < ARRAY_SIZE(channel_names) &&
-                channel_names[chan->value->channel_type]) {
-                channel_name = channel_names[chan->value->channel_type];
-            }
-
-            monitor_printf(mon, "     channel name: %s\n", channel_name);
-        }
-    }
-
-out:
-    qapi_free_SpiceInfo(info);
-}
-#endif
-
 void hmp_info_balloon(Monitor *mon, const QDict *qdict)
 {
     BalloonInfo *info;
@@ -1375,71 +1187,6 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
-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;
-
-    SetPasswordOptions opts = {
-        .password = (char *)password,
-        .has_connected = !!connected,
-    };
-
-    opts.connected = qapi_enum_parse(&SetPasswordAction_lookup, connected,
-                                     SET_PASSWORD_ACTION_KEEP, &err);
-    if (err) {
-        goto out;
-    }
-
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
-    if (err) {
-        goto out;
-    }
-
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
-        opts.u.vnc.has_display = !!display;
-        opts.u.vnc.display = (char *)display;
-    }
-
-    qmp_set_password(&opts, &err);
-
-out:
-    hmp_handle_error(mon, err);
-}
-
-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;
-
-    ExpirePasswordOptions opts = {
-        .time = (char *)whenstr,
-    };
-
-    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
-                                    DISPLAY_PROTOCOL_VNC, &err);
-    if (err) {
-        goto out;
-    }
-
-    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
-        opts.u.vnc.has_display = !!display;
-        opts.u.vnc.display = (char *)display;
-    }
-
-    qmp_expire_password(&opts, &err);
-
-out:
-    hmp_handle_error(mon, err);
-}
-
-
 #ifdef CONFIG_VNC
 static void hmp_change_read_arg(void *opaque, const char *password,
                                 void *readline_opaque)
@@ -1637,95 +1384,6 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
-void hmp_sendkey(Monitor *mon, const QDict *qdict)
-{
-    const char *keys = qdict_get_str(qdict, "keys");
-    KeyValue *v = NULL;
-    KeyValueList *head = NULL, **tail = &head;
-    int has_hold_time = qdict_haskey(qdict, "hold-time");
-    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
-    Error *err = NULL;
-    const char *separator;
-    int keyname_len;
-
-    while (1) {
-        separator = qemu_strchrnul(keys, '-');
-        keyname_len = separator - keys;
-
-        /* Be compatible with old interface, convert user inputted "<" */
-        if (keys[0] == '<' && keyname_len == 1) {
-            keys = "less";
-            keyname_len = 4;
-        }
-
-        v = g_malloc0(sizeof(*v));
-
-        if (strstart(keys, "0x", NULL)) {
-            const char *endp;
-            unsigned long value;
-
-            if (qemu_strtoul(keys, &endp, 0, &value) < 0
-                || value >= INT_MAX) {
-                goto err_out;
-            }
-            assert(endp <= keys + keyname_len);
-            if (endp != keys + keyname_len) {
-                goto err_out;
-            }
-            v->type = KEY_VALUE_KIND_NUMBER;
-            v->u.number.data = value;
-        } else {
-            int idx = index_from_key(keys, keyname_len);
-            if (idx == Q_KEY_CODE__MAX) {
-                goto err_out;
-            }
-            v->type = KEY_VALUE_KIND_QCODE;
-            v->u.qcode.data = idx;
-        }
-        QAPI_LIST_APPEND(tail, v);
-        v = NULL;
-
-        if (!*separator) {
-            break;
-        }
-        keys = separator + 1;
-    }
-
-    qmp_send_key(head, has_hold_time, hold_time, &err);
-    hmp_handle_error(mon, err);
-
-out:
-    qapi_free_KeyValue(v);
-    qapi_free_KeyValueList(head);
-    return;
-
-err_out:
-    monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys);
-    goto out;
-}
-
-void coroutine_fn
-hmp_screendump(Monitor *mon, const QDict *qdict)
-{
-    const char *filename = qdict_get_str(qdict, "filename");
-    const char *id = qdict_get_try_str(qdict, "device");
-    int64_t head = qdict_get_try_int(qdict, "head", 0);
-    const char *input_format  = qdict_get_try_str(qdict, "format");
-    Error *err = NULL;
-    ImageFormat format;
-
-    format = qapi_enum_parse(&ImageFormat_lookup, input_format,
-                              IMAGE_FORMAT_PPM, &err);
-    if (err) {
-        goto end;
-    }
-
-    qmp_screendump(filename, id != NULL, id, id != NULL, head,
-                   input_format != NULL, format, &err);
-end:
-    hmp_handle_error(mon, err);
-}
-
 void hmp_chardev_add(Monitor *mon, const QDict *qdict)
 {
     const char *args = qdict_get_str(qdict, "args");
diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
new file mode 100644
index 0000000000..af290da9e1
--- /dev/null
+++ b/ui/ui-hmp-cmds.c
@@ -0,0 +1,360 @@
+/*
+ * Human Monitor Interface commands
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#ifdef CONFIG_SPICE
+#include <spice/enums.h>
+#endif
+#include "monitor/hmp.h"
+#include "monitor/monitor.h"
+#include "qapi/qapi-commands-ui.h"
+#include "qapi/qmp/qdict.h"
+#include "qemu/cutils.h"
+#include "ui/console.h"
+
+void hmp_info_mice(Monitor *mon, const QDict *qdict)
+{
+    MouseInfoList *mice_list, *mouse;
+
+    mice_list = qmp_query_mice(NULL);
+    if (!mice_list) {
+        monitor_printf(mon, "No mouse devices connected\n");
+        return;
+    }
+
+    for (mouse = mice_list; mouse; mouse = mouse->next) {
+        monitor_printf(mon, "%c Mouse #%" PRId64 ": %s%s\n",
+                       mouse->value->current ? '*' : ' ',
+                       mouse->value->index, mouse->value->name,
+                       mouse->value->absolute ? " (absolute)" : "");
+    }
+
+    qapi_free_MouseInfoList(mice_list);
+}
+
+#ifdef CONFIG_VNC
+/* Helper for hmp_info_vnc_clients, _servers */
+static void hmp_info_VncBasicInfo(Monitor *mon, VncBasicInfo *info,
+                                  const char *name)
+{
+    monitor_printf(mon, "  %s: %s:%s (%s%s)\n",
+                   name,
+                   info->host,
+                   info->service,
+                   NetworkAddressFamily_str(info->family),
+                   info->websocket ? " (Websocket)" : "");
+}
+
+/* Helper displaying and auth and crypt info */
+static void hmp_info_vnc_authcrypt(Monitor *mon, const char *indent,
+                                   VncPrimaryAuth auth,
+                                   VncVencryptSubAuth *vencrypt)
+{
+    monitor_printf(mon, "%sAuth: %s (Sub: %s)\n", indent,
+                   VncPrimaryAuth_str(auth),
+                   vencrypt ? VncVencryptSubAuth_str(*vencrypt) : "none");
+}
+
+static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
+{
+    while (client) {
+        VncClientInfo *cinfo = client->value;
+
+        hmp_info_VncBasicInfo(mon, qapi_VncClientInfo_base(cinfo), "Client");
+        monitor_printf(mon, "    x509_dname: %s\n",
+                       cinfo->has_x509_dname ?
+                       cinfo->x509_dname : "none");
+        monitor_printf(mon, "    sasl_username: %s\n",
+                       cinfo->has_sasl_username ?
+                       cinfo->sasl_username : "none");
+
+        client = client->next;
+    }
+}
+
+static void hmp_info_vnc_servers(Monitor *mon, VncServerInfo2List *server)
+{
+    while (server) {
+        VncServerInfo2 *sinfo = server->value;
+        hmp_info_VncBasicInfo(mon, qapi_VncServerInfo2_base(sinfo), "Server");
+        hmp_info_vnc_authcrypt(mon, "    ", sinfo->auth,
+                               sinfo->has_vencrypt ? &sinfo->vencrypt : NULL);
+        server = server->next;
+    }
+}
+
+void hmp_info_vnc(Monitor *mon, const QDict *qdict)
+{
+    VncInfo2List *info2l, *info2l_head;
+    Error *err = NULL;
+
+    info2l = qmp_query_vnc_servers(&err);
+    info2l_head = info2l;
+    if (hmp_handle_error(mon, err)) {
+        return;
+    }
+    if (!info2l) {
+        monitor_printf(mon, "None\n");
+        return;
+    }
+
+    while (info2l) {
+        VncInfo2 *info = info2l->value;
+        monitor_printf(mon, "%s:\n", info->id);
+        hmp_info_vnc_servers(mon, info->server);
+        hmp_info_vnc_clients(mon, info->clients);
+        if (!info->server) {
+            /*
+             * The server entry displays its auth, we only need to
+             * display in the case of 'reverse' connections where
+             * there's no server.
+             */
+            hmp_info_vnc_authcrypt(mon, "  ", info->auth,
+                               info->has_vencrypt ? &info->vencrypt : NULL);
+        }
+        if (info->has_display) {
+            monitor_printf(mon, "  Display: %s\n", info->display);
+        }
+        info2l = info2l->next;
+    }
+
+    qapi_free_VncInfo2List(info2l_head);
+
+}
+#endif
+
+#ifdef CONFIG_SPICE
+void hmp_info_spice(Monitor *mon, const QDict *qdict)
+{
+    SpiceChannelList *chan;
+    SpiceInfo *info;
+    const char *channel_name;
+    const char * const channel_names[] = {
+        [SPICE_CHANNEL_MAIN] = "main",
+        [SPICE_CHANNEL_DISPLAY] = "display",
+        [SPICE_CHANNEL_INPUTS] = "inputs",
+        [SPICE_CHANNEL_CURSOR] = "cursor",
+        [SPICE_CHANNEL_PLAYBACK] = "playback",
+        [SPICE_CHANNEL_RECORD] = "record",
+        [SPICE_CHANNEL_TUNNEL] = "tunnel",
+        [SPICE_CHANNEL_SMARTCARD] = "smartcard",
+        [SPICE_CHANNEL_USBREDIR] = "usbredir",
+        [SPICE_CHANNEL_PORT] = "port",
+    };
+
+    info = qmp_query_spice(NULL);
+
+    if (!info->enabled) {
+        monitor_printf(mon, "Server: disabled\n");
+        goto out;
+    }
+
+    monitor_printf(mon, "Server:\n");
+    if (info->has_port) {
+        monitor_printf(mon, "     address: %s:%" PRId64 "\n",
+                       info->host, info->port);
+    }
+    if (info->has_tls_port) {
+        monitor_printf(mon, "     address: %s:%" PRId64 " [tls]\n",
+                       info->host, info->tls_port);
+    }
+    monitor_printf(mon, "    migrated: %s\n",
+                   info->migrated ? "true" : "false");
+    monitor_printf(mon, "        auth: %s\n", info->auth);
+    monitor_printf(mon, "    compiled: %s\n", info->compiled_version);
+    monitor_printf(mon, "  mouse-mode: %s\n",
+                   SpiceQueryMouseMode_str(info->mouse_mode));
+
+    if (!info->has_channels || info->channels == NULL) {
+        monitor_printf(mon, "Channels: none\n");
+    } else {
+        for (chan = info->channels; chan; chan = chan->next) {
+            monitor_printf(mon, "Channel:\n");
+            monitor_printf(mon, "     address: %s:%s%s\n",
+                           chan->value->host, chan->value->port,
+                           chan->value->tls ? " [tls]" : "");
+            monitor_printf(mon, "     session: %" PRId64 "\n",
+                           chan->value->connection_id);
+            monitor_printf(mon, "     channel: %" PRId64 ":%" PRId64 "\n",
+                           chan->value->channel_type, chan->value->channel_id);
+
+            channel_name = "unknown";
+            if (chan->value->channel_type > 0 &&
+                chan->value->channel_type < ARRAY_SIZE(channel_names) &&
+                channel_names[chan->value->channel_type]) {
+                channel_name = channel_names[chan->value->channel_type];
+            }
+
+            monitor_printf(mon, "     channel name: %s\n", channel_name);
+        }
+    }
+
+out:
+    qapi_free_SpiceInfo(info);
+}
+#endif
+
+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;
+
+    SetPasswordOptions opts = {
+        .password = (char *)password,
+        .has_connected = !!connected,
+    };
+
+    opts.connected = qapi_enum_parse(&SetPasswordAction_lookup, connected,
+                                     SET_PASSWORD_ACTION_KEEP, &err);
+    if (err) {
+        goto out;
+    }
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        goto out;
+    }
+
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = (char *)display;
+    }
+
+    qmp_set_password(&opts, &err);
+
+out:
+    hmp_handle_error(mon, err);
+}
+
+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;
+
+    ExpirePasswordOptions opts = {
+        .time = (char *)whenstr,
+    };
+
+    opts.protocol = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
+                                    DISPLAY_PROTOCOL_VNC, &err);
+    if (err) {
+        goto out;
+    }
+
+    if (opts.protocol == DISPLAY_PROTOCOL_VNC) {
+        opts.u.vnc.has_display = !!display;
+        opts.u.vnc.display = (char *)display;
+    }
+
+    qmp_expire_password(&opts, &err);
+
+out:
+    hmp_handle_error(mon, err);
+}
+
+void hmp_sendkey(Monitor *mon, const QDict *qdict)
+{
+    const char *keys = qdict_get_str(qdict, "keys");
+    KeyValue *v = NULL;
+    KeyValueList *head = NULL, **tail = &head;
+    int has_hold_time = qdict_haskey(qdict, "hold-time");
+    int hold_time = qdict_get_try_int(qdict, "hold-time", -1);
+    Error *err = NULL;
+    const char *separator;
+    int keyname_len;
+
+    while (1) {
+        separator = qemu_strchrnul(keys, '-');
+        keyname_len = separator - keys;
+
+        /* Be compatible with old interface, convert user inputted "<" */
+        if (keys[0] == '<' && keyname_len == 1) {
+            keys = "less";
+            keyname_len = 4;
+        }
+
+        v = g_malloc0(sizeof(*v));
+
+        if (strstart(keys, "0x", NULL)) {
+            const char *endp;
+            unsigned long value;
+
+            if (qemu_strtoul(keys, &endp, 0, &value) < 0
+                || value >= INT_MAX) {
+                goto err_out;
+            }
+            assert(endp <= keys + keyname_len);
+            if (endp != keys + keyname_len) {
+                goto err_out;
+            }
+            v->type = KEY_VALUE_KIND_NUMBER;
+            v->u.number.data = value;
+        } else {
+            int idx = index_from_key(keys, keyname_len);
+            if (idx == Q_KEY_CODE__MAX) {
+                goto err_out;
+            }
+            v->type = KEY_VALUE_KIND_QCODE;
+            v->u.qcode.data = idx;
+        }
+        QAPI_LIST_APPEND(tail, v);
+        v = NULL;
+
+        if (!*separator) {
+            break;
+        }
+        keys = separator + 1;
+    }
+
+    qmp_send_key(head, has_hold_time, hold_time, &err);
+    hmp_handle_error(mon, err);
+
+out:
+    qapi_free_KeyValue(v);
+    qapi_free_KeyValueList(head);
+    return;
+
+err_out:
+    monitor_printf(mon, "invalid parameter: %.*s\n", keyname_len, keys);
+    goto out;
+}
+
+void coroutine_fn
+hmp_screendump(Monitor *mon, const QDict *qdict)
+{
+    const char *filename = qdict_get_str(qdict, "filename");
+    const char *id = qdict_get_try_str(qdict, "device");
+    int64_t head = qdict_get_try_int(qdict, "head", 0);
+    const char *input_format  = qdict_get_try_str(qdict, "format");
+    Error *err = NULL;
+    ImageFormat format;
+
+    format = qapi_enum_parse(&ImageFormat_lookup, input_format,
+                              IMAGE_FORMAT_PPM, &err);
+    if (err) {
+        goto end;
+    }
+
+    qmp_screendump(filename, id != NULL, id, id != NULL, head,
+                   input_format != NULL, format, &err);
+end:
+    hmp_handle_error(mon, err);
+}
diff --git a/ui/meson.build b/ui/meson.build
index 9194ea335b..612ea2325b 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -14,6 +14,7 @@ softmmu_ss.add(files(
   'kbd-state.c',
   'keymaps.c',
   'qemu-pixman.c',
+  'ui-hmp-cmds.c',
   'ui-qmp-cmds.c',
   'util.c',
 ))
-- 
2.37.3

Re: [PATCH 6/9] ui: Move HMP commands from monitor to new ui/ui-hmp-cmds.c

[Philippe Mathieu-Daudé]

On 1/12/22 07:13, Markus Armbruster wrote:
> This moves these commands from MAINTAINERS section "Human
> Monitor (HMP)" to "Graphics".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   monitor/hmp-cmds.c | 342 ------------------------------------------
>   ui/ui-hmp-cmds.c   | 360 +++++++++++++++++++++++++++++++++++++++++++++
>   ui/meson.build     |   1 +
>   3 files changed, 361 insertions(+), 342 deletions(-)
>   create mode 100644 ui/ui-hmp-cmds.c


> +#ifdef CONFIG_SPICE
> +void hmp_info_spice(Monitor *mon, const QDict *qdict)
> +{
> +    SpiceChannelList *chan;
> +    SpiceInfo *info;
> +    const char *channel_name;
> +    const char * const channel_names[] = {

Can be static (pre-existing).

> +        [SPICE_CHANNEL_MAIN] = "main",
> +        [SPICE_CHANNEL_DISPLAY] = "display",
> +        [SPICE_CHANNEL_INPUTS] = "inputs",
> +        [SPICE_CHANNEL_CURSOR] = "cursor",
> +        [SPICE_CHANNEL_PLAYBACK] = "playback",
> +        [SPICE_CHANNEL_RECORD] = "record",
> +        [SPICE_CHANNEL_TUNNEL] = "tunnel",
> +        [SPICE_CHANNEL_SMARTCARD] = "smartcard",
> +        [SPICE_CHANNEL_USBREDIR] = "usbredir",
> +        [SPICE_CHANNEL_PORT] = "port",
> +    };
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 6/9] ui: Move HMP commands from monitor to new ui/ui-hmp-cmds.c

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 1/12/22 07:13, Markus Armbruster wrote:
>> This moves these commands from MAINTAINERS section "Human
>> Monitor (HMP)" to "Graphics".
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   monitor/hmp-cmds.c | 342 ------------------------------------------
>>   ui/ui-hmp-cmds.c   | 360 +++++++++++++++++++++++++++++++++++++++++++++
>>   ui/meson.build     |   1 +
>>   3 files changed, 361 insertions(+), 342 deletions(-)
>>   create mode 100644 ui/ui-hmp-cmds.c
>
>
>> +#ifdef CONFIG_SPICE
>> +void hmp_info_spice(Monitor *mon, const QDict *qdict)
>> +{
>> +    SpiceChannelList *chan;
>> +    SpiceInfo *info;
>> +    const char *channel_name;
>> +    const char * const channel_names[] = {
>
> Can be static (pre-existing).

Separate patch, to keep the code motion pure.

>> +        [SPICE_CHANNEL_MAIN] = "main",
>> +        [SPICE_CHANNEL_DISPLAY] = "display",
>> +        [SPICE_CHANNEL_INPUTS] = "inputs",
>> +        [SPICE_CHANNEL_CURSOR] = "cursor",
>> +        [SPICE_CHANNEL_PLAYBACK] = "playback",
>> +        [SPICE_CHANNEL_RECORD] = "record",
>> +        [SPICE_CHANNEL_TUNNEL] = "tunnel",
>> +        [SPICE_CHANNEL_SMARTCARD] = "smartcard",
>> +        [SPICE_CHANNEL_USBREDIR] = "usbredir",
>> +        [SPICE_CHANNEL_PORT] = "port",
>> +    };
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

Re: [PATCH 6/9] ui: Move HMP commands from monitor to new ui/ui-hmp-cmds.c

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:08AM +0100, Markus Armbruster wrote:
> This moves these commands from MAINTAINERS section "Human
> Monitor (HMP)" to "Graphics".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/hmp-cmds.c | 342 ------------------------------------------
>  ui/ui-hmp-cmds.c   | 360 +++++++++++++++++++++++++++++++++++++++++++++
>  ui/meson.build     |   1 +
>  3 files changed, 361 insertions(+), 342 deletions(-)
>  create mode 100644 ui/ui-hmp-cmds.c

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

[PATCH 7/9] ui: Improve "change vnc" error reporting

[Markus Armbruster]

Switch from monitor_printf() to error_setg() and hmp_handle_error().
This makes "this is an error" more obvious both in the source and in
the monitor, where hmp_handle_error() prefixes the message with
"Error: ".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor/hmp-cmds.c |  8 ++++----
 ui/ui-hmp-cmds.c   | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index f0f7b74fb3..8542eee3d4 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1209,9 +1209,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 #ifdef CONFIG_VNC
     if (strcmp(device, "vnc") == 0) {
         if (read_only) {
-            monitor_printf(mon,
-                           "Parameter 'read-only-mode' is invalid for VNC\n");
-            return;
+            error_setg(&err, "Parameter 'read-only-mode' is invalid for VNC");
+            goto end;
         }
         if (strcmp(target, "passwd") == 0 ||
             strcmp(target, "password") == 0) {
@@ -1223,7 +1222,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
                 qmp_change_vnc_password(arg, &err);
             }
         } else {
-            monitor_printf(mon, "Expected 'password' after 'vnc'\n");
+            error_setg(&err, "Expected 'password' after 'vnc'");
+            goto end;
         }
     } else
 #endif
diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
index af290da9e1..90a4f86f25 100644
--- a/ui/ui-hmp-cmds.c
+++ b/ui/ui-hmp-cmds.c
@@ -270,6 +270,28 @@ out:
     hmp_handle_error(mon, err);
 }
 
+void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
+                    const char *arg, const char *read_only, bool force,
+                    Error **errp)
+{
+    if (read_only) {
+        error_setg(mon, "Parameter 'read-only-mode' is invalid for VNC");
+        return;
+    }
+    if (strcmp(target, "passwd") == 0 ||
+        strcmp(target, "password") == 0) {
+        if (!arg) {
+            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
+            monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
+            return;
+        } else {
+            qmp_change_vnc_password(arg, &err);
+        }
+    } else {
+        monitor_printf(mon, "Expected 'password' after 'vnc'\n");
+    }
+}
+
 void hmp_sendkey(Monitor *mon, const QDict *qdict)
 {
     const char *keys = qdict_get_str(qdict, "keys");
-- 
2.37.3

Re: [PATCH 7/9] ui: Improve "change vnc" error reporting

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:09AM +0100, Markus Armbruster wrote:
> Switch from monitor_printf() to error_setg() and hmp_handle_error().
> This makes "this is an error" more obvious both in the source and in
> the monitor, where hmp_handle_error() prefixes the message with
> "Error: ".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor/hmp-cmds.c |  8 ++++----
>  ui/ui-hmp-cmds.c   | 22 ++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index f0f7b74fb3..8542eee3d4 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1209,9 +1209,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>  #ifdef CONFIG_VNC
>      if (strcmp(device, "vnc") == 0) {
>          if (read_only) {
> -            monitor_printf(mon,
> -                           "Parameter 'read-only-mode' is invalid for VNC\n");
> -            return;
> +            error_setg(&err, "Parameter 'read-only-mode' is invalid for VNC");
> +            goto end;
>          }
>          if (strcmp(target, "passwd") == 0 ||
>              strcmp(target, "password") == 0) {
> @@ -1223,7 +1222,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>                  qmp_change_vnc_password(arg, &err);
>              }
>          } else {
> -            monitor_printf(mon, "Expected 'password' after 'vnc'\n");
> +            error_setg(&err, "Expected 'password' after 'vnc'");
> +            goto end;
>          }
>      } else
>  #endif

Upto this point

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
> index af290da9e1..90a4f86f25 100644
> --- a/ui/ui-hmp-cmds.c
> +++ b/ui/ui-hmp-cmds.c
> @@ -270,6 +270,28 @@ out:
>      hmp_handle_error(mon, err);
>  }
>  
> +void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
> +                    const char *arg, const char *read_only, bool force,
> +                    Error **errp)
> +{
> +    if (read_only) {
> +        error_setg(mon, "Parameter 'read-only-mode' is invalid for VNC");
> +        return;
> +    }
> +    if (strcmp(target, "passwd") == 0 ||
> +        strcmp(target, "password") == 0) {
> +        if (!arg) {
> +            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
> +            monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
> +            return;
> +        } else {
> +            qmp_change_vnc_password(arg, &err);
> +        }
> +    } else {
> +        monitor_printf(mon, "Expected 'password' after 'vnc'\n");
> +    }
> +}
> +

This chunk ought to be in the next patch IIUC

>  void hmp_sendkey(Monitor *mon, const QDict *qdict)
>  {
>      const char *keys = qdict_get_str(qdict, "keys");
> -- 
> 2.37.3
> 
> 

With 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 7/9] ui: Improve "change vnc" error reporting

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Dec 01, 2022 at 07:13:09AM +0100, Markus Armbruster wrote:
>> Switch from monitor_printf() to error_setg() and hmp_handle_error().
>> This makes "this is an error" more obvious both in the source and in
>> the monitor, where hmp_handle_error() prefixes the message with
>> "Error: ".
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor/hmp-cmds.c |  8 ++++----
>>  ui/ui-hmp-cmds.c   | 22 ++++++++++++++++++++++
>>  2 files changed, 26 insertions(+), 4 deletions(-)
>> 
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index f0f7b74fb3..8542eee3d4 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -1209,9 +1209,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>>  #ifdef CONFIG_VNC
>>      if (strcmp(device, "vnc") == 0) {
>>          if (read_only) {
>> -            monitor_printf(mon,
>> -                           "Parameter 'read-only-mode' is invalid for VNC\n");
>> -            return;
>> +            error_setg(&err, "Parameter 'read-only-mode' is invalid for VNC");
>> +            goto end;
>>          }
>>          if (strcmp(target, "passwd") == 0 ||
>>              strcmp(target, "password") == 0) {
>> @@ -1223,7 +1222,8 @@ void hmp_change(Monitor *mon, const QDict *qdict)
>>                  qmp_change_vnc_password(arg, &err);
>>              }
>>          } else {
>> -            monitor_printf(mon, "Expected 'password' after 'vnc'\n");
>> +            error_setg(&err, "Expected 'password' after 'vnc'");
>> +            goto end;
>>          }
>>      } else
>>  #endif
>
> Upto this point
>
>   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
>> diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
>> index af290da9e1..90a4f86f25 100644
>> --- a/ui/ui-hmp-cmds.c
>> +++ b/ui/ui-hmp-cmds.c
>> @@ -270,6 +270,28 @@ out:
>>      hmp_handle_error(mon, err);
>>  }
>>  
>> +void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
>> +                    const char *arg, const char *read_only, bool force,
>> +                    Error **errp)
>> +{
>> +    if (read_only) {
>> +        error_setg(mon, "Parameter 'read-only-mode' is invalid for VNC");
>> +        return;
>> +    }
>> +    if (strcmp(target, "passwd") == 0 ||
>> +        strcmp(target, "password") == 0) {
>> +        if (!arg) {
>> +            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
>> +            monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
>> +            return;
>> +        } else {
>> +            qmp_change_vnc_password(arg, &err);
>> +        }
>> +    } else {
>> +        monitor_printf(mon, "Expected 'password' after 'vnc'\n");
>> +    }
>> +}
>> +
>
> This chunk ought to be in the next patch IIUC

Oops!  Thank you!

>>  void hmp_sendkey(Monitor *mon, const QDict *qdict)
>>  {
>>      const char *keys = qdict_get_str(qdict, "keys");
>> -- 
>> 2.37.3
>> 
>> 
>
> With regards,
> Daniel

[PATCH 8/9] ui: Factor out hmp_change_vnc(), and move to ui/ui-hmp-cmds.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/monitor/hmp.h |  5 +++++
 monitor/hmp-cmds.c    | 28 +---------------------------
 ui/ui-hmp-cmds.c      | 19 +++++++++++++++----
 3 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index dfbc0c9a2f..992d91f181 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -73,6 +73,11 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict);
 void hmp_set_password(Monitor *mon, const QDict *qdict);
 void hmp_expire_password(Monitor *mon, const QDict *qdict);
 void hmp_change(Monitor *mon, const QDict *qdict);
+#ifdef CONFIG_VNC
+void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
+                    const char *arg, const char *read_only, bool force,
+                    Error **errp);
+#endif
 void hmp_migrate(Monitor *mon, const QDict *qdict);
 void hmp_device_add(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 8542eee3d4..78bcdede85 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -42,7 +42,6 @@
 #include "qapi/qapi-commands-run-state.h"
 #include "qapi/qapi-commands-stats.h"
 #include "qapi/qapi-commands-tpm.h"
-#include "qapi/qapi-commands-ui.h"
 #include "qapi/qapi-commands-virtio.h"
 #include "qapi/qapi-visit-virtio.h"
 #include "qapi/qapi-visit-net.h"
@@ -1187,15 +1186,6 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
-#ifdef CONFIG_VNC
-static void hmp_change_read_arg(void *opaque, const char *password,
-                                void *readline_opaque)
-{
-    qmp_change_vnc_password(password, NULL);
-    monitor_read_command(opaque, 1);
-}
-#endif
-
 void hmp_change(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -1208,23 +1198,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 
 #ifdef CONFIG_VNC
     if (strcmp(device, "vnc") == 0) {
-        if (read_only) {
-            error_setg(&err, "Parameter 'read-only-mode' is invalid for VNC");
-            goto end;
-        }
-        if (strcmp(target, "passwd") == 0 ||
-            strcmp(target, "password") == 0) {
-            if (!arg) {
-                MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
-                monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
-                return;
-            } else {
-                qmp_change_vnc_password(arg, &err);
-            }
-        } else {
-            error_setg(&err, "Expected 'password' after 'vnc'");
-            goto end;
-        }
+        hmp_change_vnc(mon, device, target, arg, read_only, force, &err);
     } else
 #endif
     {
diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
index 90a4f86f25..b1b18d5a5d 100644
--- a/ui/ui-hmp-cmds.c
+++ b/ui/ui-hmp-cmds.c
@@ -18,7 +18,8 @@
 #include <spice/enums.h>
 #endif
 #include "monitor/hmp.h"
-#include "monitor/monitor.h"
+#include "monitor/monitor-internal.h"
+#include "qapi/error.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/cutils.h"
@@ -270,12 +271,20 @@ out:
     hmp_handle_error(mon, err);
 }
 
+#ifdef CONFIG_VNC
+static void hmp_change_read_arg(void *opaque, const char *password,
+                                void *readline_opaque)
+{
+    qmp_change_vnc_password(password, NULL);
+    monitor_read_command(opaque, 1);
+}
+
 void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
                     const char *arg, const char *read_only, bool force,
                     Error **errp)
 {
     if (read_only) {
-        error_setg(mon, "Parameter 'read-only-mode' is invalid for VNC");
+        error_setg(errp, "Parameter 'read-only-mode' is invalid for VNC");
         return;
     }
     if (strcmp(target, "passwd") == 0 ||
@@ -285,12 +294,14 @@ void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
             monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
             return;
         } else {
-            qmp_change_vnc_password(arg, &err);
+            qmp_change_vnc_password(arg, errp);
         }
     } else {
-        monitor_printf(mon, "Expected 'password' after 'vnc'\n");
+        error_setg(errp, "Expected 'password' after 'vnc'");
+        return;
     }
 }
+#endif
 
 void hmp_sendkey(Monitor *mon, const QDict *qdict)
 {
-- 
2.37.3

Re: [PATCH 8/9] ui: Factor out hmp_change_vnc(), and move to ui/ui-hmp-cmds.c

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:10AM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/monitor/hmp.h |  5 +++++
>  monitor/hmp-cmds.c    | 28 +---------------------------
>  ui/ui-hmp-cmds.c      | 19 +++++++++++++++----
>  3 files changed, 21 insertions(+), 31 deletions(-)

If you pull in the hmp_change_vnc impl from the previous patch

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



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

[PATCH 9/9] ui: Reduce nesting in hmp_change_vnc() slightly

[Markus Armbruster]

Transform

    if (good) {
        do stuff
    } else {
        handle error
    }

to

    if (!good) {
        handle error
	return;
    }
    do stuff

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/ui-hmp-cmds.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/ui/ui-hmp-cmds.c b/ui/ui-hmp-cmds.c
index b1b18d5a5d..31778122d3 100644
--- a/ui/ui-hmp-cmds.c
+++ b/ui/ui-hmp-cmds.c
@@ -287,19 +287,16 @@ void hmp_change_vnc(Monitor *mon, const char *device, const char *target,
         error_setg(errp, "Parameter 'read-only-mode' is invalid for VNC");
         return;
     }
-    if (strcmp(target, "passwd") == 0 ||
-        strcmp(target, "password") == 0) {
-        if (!arg) {
-            MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
-            monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
-            return;
-        } else {
-            qmp_change_vnc_password(arg, errp);
-        }
-    } else {
+    if (strcmp(target, "passwd") && strcmp(target, "password")) {
         error_setg(errp, "Expected 'password' after 'vnc'");
         return;
     }
+    if (!arg) {
+        MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
+        monitor_read_password(hmp_mon, hmp_change_read_arg, NULL);
+    } else {
+        qmp_change_vnc_password(arg, errp);
+    }
 }
 #endif
 
-- 
2.37.3

Re: [PATCH 9/9] ui: Reduce nesting in hmp_change_vnc() slightly

[Philippe Mathieu-Daudé]

On 1/12/22 07:13, Markus Armbruster wrote:
> Transform
> 
>      if (good) {
>          do stuff
>      } else {
>          handle error
>      }
> 
> to
> 
>      if (!good) {
>          handle error
> 	return;
>      }
>      do stuff
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   ui/ui-hmp-cmds.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 9/9] ui: Reduce nesting in hmp_change_vnc() slightly

[Daniel P. Berrangé]

On Thu, Dec 01, 2022 at 07:13:11AM +0100, Markus Armbruster wrote:
> Transform
> 
>     if (good) {
>         do stuff
>     } else {
>         handle error
>     }
> 
> to
> 
>     if (!good) {
>         handle error
> 	return;
>     }
>     do stuff
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/ui-hmp-cmds.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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