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

2016-09-29 Thread Chen Hanxiao 


At 2016-09-29 14:27:56, "Michal Privoznik"  wrote:
>On 28.09.2016 15:31, Chen Hanxiao wrote:
>> From: Chen Hanxiao 
>> 
>> 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 
>> ---
>> 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.

We had code:
  if (type && STRNEQ(type, scheme[iter])) 
  continue;
in the front of the loop.
Maybe we should ignore --type if --all specified.

[...]

>
>Almost. You forgot to update the list of arguments:
>
>-=item B I [I<--include-password>] [[I<--type>] B]
>+=item B I [I<--include-password>] [[I<--type>] B] 
>[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.
>

I'll send a v3 patch to address these issues for the next version of libvirt.

Regards,
- Chen

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-09-29 Thread Michal Privoznik 
On 28.09.2016 15:31, Chen Hanxiao wrote:
> From: Chen Hanxiao 
> 
> 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 
> ---
> 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_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();
>  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 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 I [I<--include-password>] [[I<--type>] B]
+=item B I [I<--include-password>] [[I<--type>] B] 
[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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2016-09-28 Thread Chen Hanxiao 
From: Chen Hanxiao 

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 
---
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(-)

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}
 };
 
@@ -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_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();
 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 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.
 
 =item B I
 
-- 
1.8.3.1


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list