[libvirt] [PATCH] virsh domdisplay: introduce '--all' for showing all possible graphical display

[Michal Privoznik]

On 22.09.2016 12:43, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at gmail.com>
> 
> 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 at gmail.com>
> ---
>  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