* Markus Armbruster (arm...@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> writes:
>
> > From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
