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

On 28.09.2016 15:31, Chen Hanxiao wrote: > From: Chen Hanxiao <chenhanxiao(a)gmail.com&gt; > > 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(a)gmail.com&gt; > --- > 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.

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.