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

[Michal Privoznik]

On 28.09.2016 15:31, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao at 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 at 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