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