[Qemu-devel] [PATCH] hmp: Update info vnc

[Qemu-devel] [PATCH] hmp: Update info vnc

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The QMP query-vnc interfaces have gained a lot more information that
the HMP interfaces hasn't got yet. Update it.

Note the output format has changed, but this is HMP so that's OK.

In particular, this now includes client information for reverse
connections:

-vnc :0
(qemu) info vnc
default:
  Server: 0.0.0.0:5900 (ipv4)
    Auth: none (Sub: none)
  Auth: none (Sub: none)

  (Now connect a client)

(qemu) info vnc
default:
  Server: 0.0.0.0:5900 (ipv4)
    Auth: none (Sub: none)
  Client: 127.0.0.1:51828 (ipv4)
    x509_dname: none
    username: none
  Auth: none (Sub: none)

-vnc localhost:7000,reverse
(qemu) info vnc
default:
  Client: ::1:7000 (ipv6)
    x509_dname: none
    username: none
  Auth: none (Sub: none)

-vnc :1,password,id=rev -vnc localhost:7000,reverse
(qemu) info vnc
default:
  Client: ::1:7000 (ipv6)
    x509_dname: none
    username: none
  Auth: none (Sub: none)
rev:
  Server: 0.0.0.0:5901 (ipv4)
    Auth: vnc (Sub: none)
  Client: 127.0.0.1:53616 (ipv4)
    x509_dname: none
    username: none
  Auth: vnc (Sub: none)

This was originally RH bz 1461682

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 31 deletions(-)

diff --git a/hmp.c b/hmp.c
index dee40284c1..c11a5fe2c6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -600,50 +600,86 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
     qapi_free_BlockStatsList(stats_list);
 }
 
+/* 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_lookup[info->family],
+                   info->websocket ? " (Websocket)" : "");
+}
+
+/* Helper displaying and euth 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_lookup[auth],
+                   vencrypt ? VncVencryptSubAuth_lookup[*vencrypt] : "none");
+}
+
+static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
+{
+    while (client) {
+        VncClientInfo *cinfo = client->value;
+
+        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)cinfo, "Client");
+        monitor_printf(mon, "    x509_dname: %s\n",
+                       cinfo->x509_dname ?
+                       cinfo->x509_dname : "none");
+        monitor_printf(mon, "    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, (VncBasicInfo *)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)
 {
-    VncInfo *info;
+    VncInfo2List *info2l;
     Error *err = NULL;
-    VncClientInfoList *client;
 
-    info = qmp_query_vnc(&err);
+    info2l = qmp_query_vnc_servers(&err);
     if (err) {
         error_report_err(err);
         return;
     }
-
-    if (!info->enabled) {
-        monitor_printf(mon, "Server: disabled\n");
-        goto out;
-    }
-
-    monitor_printf(mon, "Server:\n");
-    if (info->has_host && info->has_service) {
-        monitor_printf(mon, "     address: %s:%s\n", info->host, info->service);
-    }
-    if (info->has_auth) {
-        monitor_printf(mon, "        auth: %s\n", info->auth);
+    if (!info2l) {
+        monitor_printf(mon, "None\n");
+        return;
     }
 
-    if (!info->has_clients || info->clients == NULL) {
-        monitor_printf(mon, "Client: none\n");
-    } else {
-        for (client = info->clients; client; client = client->next) {
-            monitor_printf(mon, "Client:\n");
-            monitor_printf(mon, "     address: %s:%s\n",
-                           client->value->host,
-                           client->value->service);
-            monitor_printf(mon, "  x509_dname: %s\n",
-                           client->value->x509_dname ?
-                           client->value->x509_dname : "none");
-            monitor_printf(mon, "    username: %s\n",
-                           client->value->has_sasl_username ?
-                           client->value->sasl_username : "none");
+    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);
+        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;
     }
 
-out:
-    qapi_free_VncInfo(info);
+    qapi_free_VncInfo2List(info2l);
+
 }
 
 #ifdef CONFIG_SPICE
-- 
2.13.0

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Marc-André Lureau]

----- Original Message -----
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The QMP query-vnc interfaces have gained a lot more information that
> the HMP interfaces hasn't got yet. Update it.
> 
> Note the output format has changed, but this is HMP so that's OK.
> 
> In particular, this now includes client information for reverse
> connections:
> 
> -vnc :0
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Auth: none (Sub: none)
> 
>   (Now connect a client)
> 
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Client: 127.0.0.1:51828 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> 
> -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> 
> -vnc :1,password,id=rev -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> rev:
>   Server: 0.0.0.0:5901 (ipv4)
>     Auth: vnc (Sub: none)
>   Client: 127.0.0.1:53616 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: vnc (Sub: none)
> 
> This was originally RH bz 1461682
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hmp.c | 98
>  ++++++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 67 insertions(+), 31 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index dee40284c1..c11a5fe2c6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -600,50 +600,86 @@ void hmp_info_blockstats(Monitor *mon, const QDict
> *qdict)
>      qapi_free_BlockStatsList(stats_list);
>  }
>  
> +/* 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_lookup[info->family],
> +                   info->websocket ? " (Websocket)" : "");
> +}
> +
> +/* Helper displaying and euth 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_lookup[auth],
> +                   vencrypt ? VncVencryptSubAuth_lookup[*vencrypt] :
> "none");
> +}
> +
> +static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
> +{
> +    while (client) {
> +        VncClientInfo *cinfo = client->value;
> +
> +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)cinfo, "Client");
> +        monitor_printf(mon, "    x509_dname: %s\n",
> +                       cinfo->x509_dname ?

Why not use has_x509_dname ?

> +                       cinfo->x509_dname : "none");
> +        monitor_printf(mon, "    username: %s\n",

sasl_username would be more clear

> +                       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, (VncBasicInfo *)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)
>  {
> -    VncInfo *info;
> +    VncInfo2List *info2l;
>      Error *err = NULL;
> -    VncClientInfoList *client;
>  
> -    info = qmp_query_vnc(&err);
> +    info2l = qmp_query_vnc_servers(&err);
>      if (err) {
>          error_report_err(err);
>          return;
>      }
> -
> -    if (!info->enabled) {
> -        monitor_printf(mon, "Server: disabled\n");
> -        goto out;
> -    }
> -
> -    monitor_printf(mon, "Server:\n");
> -    if (info->has_host && info->has_service) {
> -        monitor_printf(mon, "     address: %s:%s\n", info->host,
> info->service);
> -    }
> -    if (info->has_auth) {
> -        monitor_printf(mon, "        auth: %s\n", info->auth);
> +    if (!info2l) {
> +        monitor_printf(mon, "None\n");
> +        return;
>      }
>  
> -    if (!info->has_clients || info->clients == NULL) {
> -        monitor_printf(mon, "Client: none\n");
> -    } else {
> -        for (client = info->clients; client; client = client->next) {
> -            monitor_printf(mon, "Client:\n");
> -            monitor_printf(mon, "     address: %s:%s\n",
> -                           client->value->host,
> -                           client->value->service);
> -            monitor_printf(mon, "  x509_dname: %s\n",
> -                           client->value->x509_dname ?
> -                           client->value->x509_dname : "none");
> -            monitor_printf(mon, "    username: %s\n",
> -                           client->value->has_sasl_username ?
> -                           client->value->sasl_username : "none");
> +    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);
> +        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;
>      }
>  
> -out:
> -    qapi_free_VncInfo(info);
> +    qapi_free_VncInfo2List(info2l);
> +
>  }
>  
>  #ifdef CONFIG_SPICE
> --
> 2.13.0

Looks good to me otherwise
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@redhat.com) wrote:
> 
> 
> ----- Original Message -----
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The QMP query-vnc interfaces have gained a lot more information that
> > the HMP interfaces hasn't got yet. Update it.
> > 
> > Note the output format has changed, but this is HMP so that's OK.
> > 
> > In particular, this now includes client information for reverse
> > connections:
> > 
> > -vnc :0
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Auth: none (Sub: none)
> > 
> >   (Now connect a client)
> > 
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Client: 127.0.0.1:51828 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > 
> > -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > 
> > -vnc :1,password,id=rev -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > rev:
> >   Server: 0.0.0.0:5901 (ipv4)
> >     Auth: vnc (Sub: none)
> >   Client: 127.0.0.1:53616 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: vnc (Sub: none)
> > 
> > This was originally RH bz 1461682
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hmp.c | 98
> >  ++++++++++++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 67 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hmp.c b/hmp.c
> > index dee40284c1..c11a5fe2c6 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -600,50 +600,86 @@ void hmp_info_blockstats(Monitor *mon, const QDict
> > *qdict)
> >      qapi_free_BlockStatsList(stats_list);
> >  }
> >  
> > +/* 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_lookup[info->family],
> > +                   info->websocket ? " (Websocket)" : "");
> > +}
> > +
> > +/* Helper displaying and euth 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_lookup[auth],
> > +                   vencrypt ? VncVencryptSubAuth_lookup[*vencrypt] :
> > "none");
> > +}
> > +
> > +static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
> > +{
> > +    while (client) {
> > +        VncClientInfo *cinfo = client->value;
> > +
> > +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)cinfo, "Client");
> > +        monitor_printf(mon, "    x509_dname: %s\n",
> > +                       cinfo->x509_dname ?
> 
> Why not use has_x509_dname ?

Interesting; that line I just moved from the old code;  fixed.

> > +                       cinfo->x509_dname : "none");
> > +        monitor_printf(mon, "    username: %s\n",
> 
> sasl_username would be more clear

Also from the old code; also fixed.

Thanks,

Dave

> > +                       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, (VncBasicInfo *)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)
> >  {
> > -    VncInfo *info;
> > +    VncInfo2List *info2l;
> >      Error *err = NULL;
> > -    VncClientInfoList *client;
> >  
> > -    info = qmp_query_vnc(&err);
> > +    info2l = qmp_query_vnc_servers(&err);
> >      if (err) {
> >          error_report_err(err);
> >          return;
> >      }
> > -
> > -    if (!info->enabled) {
> > -        monitor_printf(mon, "Server: disabled\n");
> > -        goto out;
> > -    }
> > -
> > -    monitor_printf(mon, "Server:\n");
> > -    if (info->has_host && info->has_service) {
> > -        monitor_printf(mon, "     address: %s:%s\n", info->host,
> > info->service);
> > -    }
> > -    if (info->has_auth) {
> > -        monitor_printf(mon, "        auth: %s\n", info->auth);
> > +    if (!info2l) {
> > +        monitor_printf(mon, "None\n");
> > +        return;
> >      }
> >  
> > -    if (!info->has_clients || info->clients == NULL) {
> > -        monitor_printf(mon, "Client: none\n");
> > -    } else {
> > -        for (client = info->clients; client; client = client->next) {
> > -            monitor_printf(mon, "Client:\n");
> > -            monitor_printf(mon, "     address: %s:%s\n",
> > -                           client->value->host,
> > -                           client->value->service);
> > -            monitor_printf(mon, "  x509_dname: %s\n",
> > -                           client->value->x509_dname ?
> > -                           client->value->x509_dname : "none");
> > -            monitor_printf(mon, "    username: %s\n",
> > -                           client->value->has_sasl_username ?
> > -                           client->value->sasl_username : "none");
> > +    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);
> > +        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;
> >      }
> >  
> > -out:
> > -    qapi_free_VncInfo(info);
> > +    qapi_free_VncInfo2List(info2l);
> > +
> >  }
> >  
> >  #ifdef CONFIG_SPICE
> > --
> > 2.13.0
> 
> Looks good to me otherwise
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Daniel P. Berrange]

On Wed, Jul 05, 2017 at 03:43:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The QMP query-vnc interfaces have gained a lot more information that
> the HMP interfaces hasn't got yet. Update it.
> 
> Note the output format has changed, but this is HMP so that's OK.
> 
> In particular, this now includes client information for reverse
> connections:
> 
> -vnc :0
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Auth: none (Sub: none)

If you're reporting Auth: against the individual Server entry,
there's no reason to report it at the top level too. We only
keep that duplication in QMP for sake of backcompat, which
is not important for HMP

> 
>   (Now connect a client)
> 
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Client: 127.0.0.1:51828 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> 
> -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> 
> -vnc :1,password,id=rev -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> rev:
>   Server: 0.0.0.0:5901 (ipv4)
>     Auth: vnc (Sub: none)
>   Client: 127.0.0.1:53616 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: vnc (Sub: none)
> 
> This was originally RH bz 1461682
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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: [Qemu-devel] [PATCH] hmp: Update info vnc

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Wed, Jul 05, 2017 at 03:43:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The QMP query-vnc interfaces have gained a lot more information that
> > the HMP interfaces hasn't got yet. Update it.
> > 
> > Note the output format has changed, but this is HMP so that's OK.
> > 
> > In particular, this now includes client information for reverse
> > connections:
> > 
> > -vnc :0
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Auth: none (Sub: none)
> 
> If you're reporting Auth: against the individual Server entry,
> there's no reason to report it at the top level too. We only
> keep that duplication in QMP for sake of backcompat, which
> is not important for HMP

Where would it get reported for a 'reverse' connection then?
That has no server entry.

Dave

> > 
> >   (Now connect a client)
> > 
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Client: 127.0.0.1:51828 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > 
> > -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > 
> > -vnc :1,password,id=rev -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > rev:
> >   Server: 0.0.0.0:5901 (ipv4)
> >     Auth: vnc (Sub: none)
> >   Client: 127.0.0.1:53616 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: vnc (Sub: none)
> > 
> > This was originally RH bz 1461682
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Daniel P. Berrange]

On Wed, Jul 05, 2017 at 05:33:24PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Wed, Jul 05, 2017 at 03:43:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > The QMP query-vnc interfaces have gained a lot more information that
> > > the HMP interfaces hasn't got yet. Update it.
> > > 
> > > Note the output format has changed, but this is HMP so that's OK.
> > > 
> > > In particular, this now includes client information for reverse
> > > connections:
> > > 
> > > -vnc :0
> > > (qemu) info vnc
> > > default:
> > >   Server: 0.0.0.0:5900 (ipv4)
> > >     Auth: none (Sub: none)
> > >   Auth: none (Sub: none)
> > 
> > If you're reporting Auth: against the individual Server entry,
> > there's no reason to report it at the top level too. We only
> > keep that duplication in QMP for sake of backcompat, which
> > is not important for HMP
> 
> Where would it get reported for a 'reverse' connection then?
> That has no server entry.

Ah right. So lets only report it at the top level, if operating
in reverse mode.


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: [Qemu-devel] [PATCH] hmp: Update info vnc

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Wed, Jul 05, 2017 at 05:33:24PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > On Wed, Jul 05, 2017 at 03:43:02PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > The QMP query-vnc interfaces have gained a lot more information that
> > > > the HMP interfaces hasn't got yet. Update it.
> > > > 
> > > > Note the output format has changed, but this is HMP so that's OK.
> > > > 
> > > > In particular, this now includes client information for reverse
> > > > connections:
> > > > 
> > > > -vnc :0
> > > > (qemu) info vnc
> > > > default:
> > > >   Server: 0.0.0.0:5900 (ipv4)
> > > >     Auth: none (Sub: none)
> > > >   Auth: none (Sub: none)
> > > 
> > > If you're reporting Auth: against the individual Server entry,
> > > there's no reason to report it at the top level too. We only
> > > keep that duplication in QMP for sake of backcompat, which
> > > is not important for HMP
> > 
> > Where would it get reported for a 'reverse' connection then?
> > That has no server entry.
> 
> Ah right. So lets only report it at the top level, if operating
> in reverse mode.

OK, new version coming up.

Dave

> 
> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> The QMP query-vnc interfaces have gained a lot more information that
> the HMP interfaces hasn't got yet. Update it.
>
> Note the output format has changed, but this is HMP so that's OK.
>
> In particular, this now includes client information for reverse
> connections:
>
> -vnc :0
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Auth: none (Sub: none)
>
>   (Now connect a client)
>
> (qemu) info vnc
> default:
>   Server: 0.0.0.0:5900 (ipv4)
>     Auth: none (Sub: none)
>   Client: 127.0.0.1:51828 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
>
> -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
>
> -vnc :1,password,id=rev -vnc localhost:7000,reverse
> (qemu) info vnc
> default:
>   Client: ::1:7000 (ipv6)
>     x509_dname: none
>     username: none
>   Auth: none (Sub: none)
> rev:
>   Server: 0.0.0.0:5901 (ipv4)
>     Auth: vnc (Sub: none)
>   Client: 127.0.0.1:53616 (ipv4)
>     x509_dname: none
>     username: none
>   Auth: vnc (Sub: none)
>
> This was originally RH bz 1461682
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hmp.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 67 insertions(+), 31 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index dee40284c1..c11a5fe2c6 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -600,50 +600,86 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>      qapi_free_BlockStatsList(stats_list);
>  }
>  
> +/* Helper for hmp_info_vnc_clients, _servers */

Not sure this comment is worth its keep.

> +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_lookup[info->family],
> +                   info->websocket ? " (Websocket)" : "");
> +}
> +
> +/* Helper displaying and euth and crypt info */

"euth"?  Do you mean "auth"?

Not sure this comment is worth its keep.

> +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_lookup[auth],
> +                   vencrypt ? VncVencryptSubAuth_lookup[*vencrypt] : "none");
> +}
> +
> +static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
> +{
> +    while (client) {
> +        VncClientInfo *cinfo = client->value;
> +
> +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)cinfo, "Client");

QAPI provides a type-safe conversion from type to base type:
qapi_TYPE_base().  You could write qapi_VncClientInfo_base(cinfo) here.
More conversions to base type below.

> +        monitor_printf(mon, "    x509_dname: %s\n",
> +                       cinfo->x509_dname ?
> +                       cinfo->x509_dname : "none");
> +        monitor_printf(mon, "    username: %s\n",
> +                       cinfo->has_sasl_username ?
> +                       cinfo->sasl_username : "none");

When an optional QAPI member FOO is a pointer, checking FOO instead of
has_FOO is totally fine.  But mixing the two in one breath looks odd.

I'd like to get rid of the redundant has_FOOs some day.

> +
> +        client = client->next;
> +    }

I prefer to keep loop control in one place, and I also prefer not to
change parameters:

       for (cl = client; cl; cl = cl->next) {

Matter of taste; your artistic license applies.  More of the same below.

> +}
> +
> +static void hmp_info_vnc_servers(Monitor *mon, VncServerInfo2List *server)
> +{
> +    while (server) {
> +        VncServerInfo2 *sinfo = server->value;
> +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)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)
>  {
> -    VncInfo *info;
> +    VncInfo2List *info2l;
>      Error *err = NULL;
> -    VncClientInfoList *client;
>  
> -    info = qmp_query_vnc(&err);
> +    info2l = qmp_query_vnc_servers(&err);
>      if (err) {
>          error_report_err(err);
>          return;
>      }
> -
> -    if (!info->enabled) {
> -        monitor_printf(mon, "Server: disabled\n");
> -        goto out;
> -    }
> -
> -    monitor_printf(mon, "Server:\n");
> -    if (info->has_host && info->has_service) {
> -        monitor_printf(mon, "     address: %s:%s\n", info->host, info->service);
> -    }
> -    if (info->has_auth) {
> -        monitor_printf(mon, "        auth: %s\n", info->auth);
> +    if (!info2l) {
> +        monitor_printf(mon, "None\n");
> +        return;
>      }
>  
> -    if (!info->has_clients || info->clients == NULL) {
> -        monitor_printf(mon, "Client: none\n");
> -    } else {
> -        for (client = info->clients; client; client = client->next) {
> -            monitor_printf(mon, "Client:\n");
> -            monitor_printf(mon, "     address: %s:%s\n",
> -                           client->value->host,
> -                           client->value->service);
> -            monitor_printf(mon, "  x509_dname: %s\n",
> -                           client->value->x509_dname ?
> -                           client->value->x509_dname : "none");
> -            monitor_printf(mon, "    username: %s\n",
> -                           client->value->has_sasl_username ?
> -                           client->value->sasl_username : "none");
> +    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);
> +        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;

Looks like you've squashed two things together: factoring out helpers
(without changing output), and extending output.  I'd keep them
separate.  You decide.

>      }
>  
> -out:
> -    qapi_free_VncInfo(info);
> +    qapi_free_VncInfo2List(info2l);
> +
>  }
>  
>  #ifdef CONFIG_SPICE

Re: [Qemu-devel] [PATCH] hmp: Update info vnc

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > The QMP query-vnc interfaces have gained a lot more information that
> > the HMP interfaces hasn't got yet. Update it.
> >
> > Note the output format has changed, but this is HMP so that's OK.
> >
> > In particular, this now includes client information for reverse
> > connections:
> >
> > -vnc :0
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Auth: none (Sub: none)
> >
> >   (Now connect a client)
> >
> > (qemu) info vnc
> > default:
> >   Server: 0.0.0.0:5900 (ipv4)
> >     Auth: none (Sub: none)
> >   Client: 127.0.0.1:51828 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> >
> > -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> >
> > -vnc :1,password,id=rev -vnc localhost:7000,reverse
> > (qemu) info vnc
> > default:
> >   Client: ::1:7000 (ipv6)
> >     x509_dname: none
> >     username: none
> >   Auth: none (Sub: none)
> > rev:
> >   Server: 0.0.0.0:5901 (ipv4)
> >     Auth: vnc (Sub: none)
> >   Client: 127.0.0.1:53616 (ipv4)
> >     x509_dname: none
> >     username: none
> >   Auth: vnc (Sub: none)
> >
> > This was originally RH bz 1461682
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hmp.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 67 insertions(+), 31 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index dee40284c1..c11a5fe2c6 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -600,50 +600,86 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
> >      qapi_free_BlockStatsList(stats_list);
> >  }
> >  
> > +/* Helper for hmp_info_vnc_clients, _servers */
> 
> Not sure this comment is worth its keep.

I prefer over rather than under commenting and I was worried that the
hmp_info_  prefix would make it look like another command handler.

> > +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_lookup[info->family],
> > +                   info->websocket ? " (Websocket)" : "");
> > +}
> > +
> > +/* Helper displaying and euth and crypt info */
> 
> "euth"?  Do you mean "auth"?
> 
> Not sure this comment is worth its keep.

Ah well spotted; fixed.

> > +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_lookup[auth],
> > +                   vencrypt ? VncVencryptSubAuth_lookup[*vencrypt] : "none");
> > +}
> > +
> > +static void hmp_info_vnc_clients(Monitor *mon, VncClientInfoList *client)
> > +{
> > +    while (client) {
> > +        VncClientInfo *cinfo = client->value;
> > +
> > +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)cinfo, "Client");
> 
> QAPI provides a type-safe conversion from type to base type:
> qapi_TYPE_base().  You could write qapi_VncClientInfo_base(cinfo) here.
> More conversions to base type below.

OK, used.

> > +        monitor_printf(mon, "    x509_dname: %s\n",
> > +                       cinfo->x509_dname ?
> > +                       cinfo->x509_dname : "none");
> > +        monitor_printf(mon, "    username: %s\n",
> > +                       cinfo->has_sasl_username ?
> > +                       cinfo->sasl_username : "none");
> 
> When an optional QAPI member FOO is a pointer, checking FOO instead of
> has_FOO is totally fine.  But mixing the two in one breath looks odd.

Yep, that's already fixed becased on previous reviews; that code is
just a few lines moved from the old code.

> I'd like to get rid of the redundant has_FOOs some day.
> 
> > +
> > +        client = client->next;
> > +    }
> 
> I prefer to keep loop control in one place, and I also prefer not to
> change parameters:
> 
>        for (cl = client; cl; cl = cl->next) {
> 
> Matter of taste; your artistic license applies.  More of the same below.

I'd rather leave as is; they're trivial loops.

> > +}
> > +
> > +static void hmp_info_vnc_servers(Monitor *mon, VncServerInfo2List *server)
> > +{
> > +    while (server) {
> > +        VncServerInfo2 *sinfo = server->value;
> > +        hmp_info_VncBasicInfo(mon, (VncBasicInfo *)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)
> >  {
> > -    VncInfo *info;
> > +    VncInfo2List *info2l;
> >      Error *err = NULL;
> > -    VncClientInfoList *client;
> >  
> > -    info = qmp_query_vnc(&err);
> > +    info2l = qmp_query_vnc_servers(&err);
> >      if (err) {
> >          error_report_err(err);
> >          return;
> >      }
> > -
> > -    if (!info->enabled) {
> > -        monitor_printf(mon, "Server: disabled\n");
> > -        goto out;
> > -    }
> > -
> > -    monitor_printf(mon, "Server:\n");
> > -    if (info->has_host && info->has_service) {
> > -        monitor_printf(mon, "     address: %s:%s\n", info->host, info->service);
> > -    }
> > -    if (info->has_auth) {
> > -        monitor_printf(mon, "        auth: %s\n", info->auth);
> > +    if (!info2l) {
> > +        monitor_printf(mon, "None\n");
> > +        return;
> >      }
> >  
> > -    if (!info->has_clients || info->clients == NULL) {
> > -        monitor_printf(mon, "Client: none\n");
> > -    } else {
> > -        for (client = info->clients; client; client = client->next) {
> > -            monitor_printf(mon, "Client:\n");
> > -            monitor_printf(mon, "     address: %s:%s\n",
> > -                           client->value->host,
> > -                           client->value->service);
> > -            monitor_printf(mon, "  x509_dname: %s\n",
> > -                           client->value->x509_dname ?
> > -                           client->value->x509_dname : "none");
> > -            monitor_printf(mon, "    username: %s\n",
> > -                           client->value->has_sasl_username ?
> > -                           client->value->sasl_username : "none");
> > +    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);
> > +        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;
> 
> Looks like you've squashed two things together: factoring out helpers
> (without changing output), and extending output.  I'd keep them
> separate.  You decide.

What I did was rewrite it for the new structure;  I borrowed ~ 2
statements from the old code, both of which got changed).

Dave

> >      }
> >  
> > -out:
> > -    qapi_free_VncInfo(info);
> > +    qapi_free_VncInfo2List(info2l);
> > +
> >  }
> >  
> >  #ifdef CONFIG_SPICE
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK