| Message ID | 20170705144302.32017-1-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
----- 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>
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
* 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
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
* 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
* 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
"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
* 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
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