On 28.09.2016 15:31, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

For one VM, it could had more than one graphical display. Such as we coud add both vnc and spice display to a VM.

This patch introduces '--all' for showing all possible graphical display of a active VM.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- v2: VIR_FREE befor use in loops add descriptions in virsh.pod

tools/virsh-domain.c | 15 ++++++++++++++- tools/virsh.pod | 3 ++- 2 files changed, 16 insertions(+), 2 deletions(-)

This one looks better. But I've got some points.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 3829b17..a6124b6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10648,6 +10648,10 @@ static const vshCmdOptDef opts_domdisplay[] = { .help = N_("select particular graphical display " "(e.g. \"vnc\", \"spice\", \"rdp\")") }, + {.name = "all", + .type = VSH_OT_BOOL, + .help = N_("show all possible graphical displays") + }, {.name = NULL} };

What should happen if users pass both --type and --all? In that case the semantics for --all is changed I guess and according to this code we would print all URIs for given type. However, there can be just one graphical type per domain and thus one URI.

@@ -10671,6 +10675,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) int tmp; int flags = 0; bool params = false; + bool all = vshCommandOptBool(cmd, "all"); const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)"; virSocketAddr addr;

@@ -10701,6 +10706,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) continue;

/* Create our XPATH lookup for the current display's port */ + VIR_FREE(xpath); if (virAsprintf(&xpath, xpath_fmt, scheme[iter], "@port") < 0) goto cleanup;

@@ -10733,6 +10739,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)

/* Attempt to get the listening addr if set for the current * graphics scheme */ + VIR_FREE(listen_addr); listen_addr = virXPathString(xpath, ctxt); VIR_FREE(xpath);

@@ -10788,6 +10795,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) goto cleanup;

/* Attempt to get the password */ + VIR_FREE(passwd); passwd = virXPathString(xpath, ctxt); VIR_FREE(xpath);

@@ -10840,12 +10848,17 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) }

/* Print out our full URI */ + VIR_FREE(output); output = virBufferContentAndReset(&buf); vshPrint(ctl, "%s", output);

/* We got what we came for so return successfully */ ret = true; - break; + if (!all) { + break; + } else { + vshPrint(ctl, "\n"); + } }

if (!ret) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 49abda9..6255b36 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1229,7 +1229,8 @@ Output a URI which can be used to connect to the graphical display of the domain via VNC, SPICE or RDP. The particular graphical display type can be selected using the B<type> parameter (e.g. "vnc", "spice", "rdp"). If I<--include-password> is specified, the SPICE channel password will be -included in the URI. +included in the URI. If I<--all> is specified, then all show all possible +graphical displays, for a VM could have more than one graphical displays.

Almost. You forgot to update the list of arguments: -=item B<domdisplay> I<domain> [I<--include-password>] [[I<--type>] B<type>] +=item B<domdisplay> I<domain> [I<--include-password>] [[I<--type>] B<type>] [I<--all>] Normally, I'd fix this before pushing and just point it out in review, but now we are in the freeze and this is a feature (during freeze only bugfixes can be pushed). Moreover, there's the unclear behaviour I described above. Michal