On 22.09.2016 12:43, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>
> For one VM, it could have 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 type of graphical display for a active VM.
>
> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
> ---
> tools/virsh-domain.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
You need to update the manpage too. Without it I cannot ACK this one.
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 3829b17..7194153 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}
> };
>
> @@ -10671,6 +10675,7 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
> int tmp;
> int flags = 0;
> bool params = false;
> + bool all = false;
You can initialize this variable right here:
bool all = vshCommandOptBool(cmd, "all"));
> const char *xpath_fmt = "string(/domain/devices/graphics[@type='%s']/%s)";
> virSocketAddr addr;
>
> @@ -10685,6 +10690,9 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
> if (vshCommandOptBool(cmd, "include-password"))
> flags |= VIR_DOMAIN_XML_SECURE;
>
> + if (vshCommandOptBool(cmd, "all"))
> + all = true;
> +
> if (vshCommandOptStringReq(ctl, cmd, "type", &type) < 0)
> goto cleanup;
>
> @@ -10845,7 +10853,15 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>
> /* We got what we came for so return successfully */
> ret = true;
> - break;
> + if (!all) {
> + break;
> + } else {
> + VIR_FREE(xpath);
> + VIR_FREE(passwd);
> + VIR_FREE(listen_addr);
> + VIR_FREE(output);
> + vshPrint(ctl, "\n");
> + }
I'd prefer if these are spread across the loop body. That is, just
before a variable set, it is prepended with VIR_FREE() call, e.g.
/* Print out our full URI */
VIR_FREE(output);
output = virBufferContentAndReset(&buf);
And so on. I like the idea!
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list