On 28.09.2016 15:31, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list