Some completers for libvirt related tools might want to list
domain IDs only. Just like the one I've implemented for
virt-viewer [1]. I've worked around it using some awk magic,
but if it was possible to just 'virsh list --id' then I could
drop awk.

1: https://www.redhat.com/archives/virt-tools-list/2019-May/msg00014.html

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

I just realized that I've never sent this and it was sitting in a
branch for 1.5 years. <facepalm/>

 tools/virsh-domain-monitor.c | 41 ++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index e0491d48ac..9e66d573e6 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1919,6 +1919,10 @@ static const vshCmdOptDef opts_list[] = {
      .type = VSH_OT_BOOL,
      .help = N_("list domain names only")
     },
+    {.name = "id",
+     .type = VSH_OT_BOOL,
+     .help = N_("list domain IDs only")
+    },
     {.name = "table",
      .type = VSH_OT_BOOL,
      .help = N_("list table (default)")
@@ -1945,6 +1949,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
     bool optTable = vshCommandOptBool(cmd, "table");
     bool optUUID = vshCommandOptBool(cmd, "uuid");
     bool optName = vshCommandOptBool(cmd, "name");
+    bool optID = vshCommandOptBool(cmd, "id");
     size_t i;
     char *title;
     char uuid[VIR_UUID_STRING_BUFLEN];
@@ -1988,8 +1993,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 
     VSH_EXCLUSIVE_OPTIONS("table", "name");
     VSH_EXCLUSIVE_OPTIONS("table", "uuid");
+    VSH_EXCLUSIVE_OPTIONS("table", "id");
+    VSH_EXCLUSIVE_OPTIONS("all", "id");
+    VSH_EXCLUSIVE_OPTIONS("inactive", "id");
+    VSH_EXCLUSIVE_OPTIONS("state-shutoff", "id");
 
-    if (!optUUID && !optName)
+    if (!optUUID && !optName && !optID)
         optTable = true;
 
     if (!(list = virshDomainListCollect(ctl, flags)))
@@ -2007,6 +2016,8 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
     }
 
     for (i = 0; i < list->ndomains; i++) {
+        const char *sep = "";
+
         dom = list->domains[i];
         id = virDomainGetID(dom);
         if (id != (unsigned int) -1)
@@ -2044,20 +2055,24 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
                     goto cleanup;
             }
 
-        } else if (optUUID && optName) {
-            if (virDomainGetUUIDString(dom, uuid) < 0) {
-                vshError(ctl, "%s", _("Failed to get domain's UUID"));
-                goto cleanup;
+        } else {
+            if (optUUID) {
+                if (virDomainGetUUIDString(dom, uuid) < 0) {
+                    vshError(ctl, "%s", _("Failed to get domain's UUID"));
+                    goto cleanup;
+                }
+                vshPrint(ctl, "%s", uuid);
+                sep = " ";
             }
-            vshPrint(ctl, "%-36s %-30s\n", uuid, virDomainGetName(dom));
-        } else if (optUUID) {
-            if (virDomainGetUUIDString(dom, uuid) < 0) {
-                vshError(ctl, "%s", _("Failed to get domain's UUID"));
-                goto cleanup;
+            if (optID) {
+                vshPrint(ctl, "%s%s", sep, id_buf);
+                sep = " ";
             }
-            vshPrint(ctl, "%s\n", uuid);
-        } else if (optName) {
-            vshPrint(ctl, "%s\n", virDomainGetName(dom));
+            if (optName) {
+                vshPrint(ctl, "%s%s", sep, virDomainGetName(dom));
+                sep = " ";
+            }
+            vshPrint(ctl, "\n");
         }
     }
 
-- 
2.26.2

On Tue, Oct 20, 2020 at 12:27:27 +0200, Michal Privoznik wrote:
> Some completers for libvirt related tools might want to list
> domain IDs only. Just like the one I've implemented for
> virt-viewer [1]. I've worked around it using some awk magic,
> but if it was possible to just 'virsh list --id' then I could
> drop awk.
> 
> 1: https://www.redhat.com/archives/virt-tools-list/2019-May/msg00014.html
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> I just realized that I've never sent this and it was sitting in a
> branch for 1.5 years. <facepalm/>
> 
>  tools/virsh-domain-monitor.c | 41 ++++++++++++++++++++++++------------

Missing change to docs/manpages/virsh.rst

>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index e0491d48ac..9e66d573e6 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c

[...]

> @@ -1988,8 +1993,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>  
>      VSH_EXCLUSIVE_OPTIONS("table", "name");
>      VSH_EXCLUSIVE_OPTIONS("table", "uuid");
> +    VSH_EXCLUSIVE_OPTIONS("table", "id");
> +    VSH_EXCLUSIVE_OPTIONS("all", "id");

This limits the usefulness. Any client wishing to use it would
specifically need to query just live VMs with this.

For completing something which accepts both active and inactive VM
identifiers, using this pattern would motivate callers do two calls to
get the list of VMs with ID and one without that argument, which is
inherently wrong/racy.

A solution would be that if --all --id and at least one of [--name,
--uuid] is used, id '-' is used for any inactive VM. Obviously --all
--id would print only active VMs and no '-'s

> +    VSH_EXCLUSIVE_OPTIONS("inactive", "id");
> +    VSH_EXCLUSIVE_OPTIONS("state-shutoff", "id");
>  
> -    if (!optUUID && !optName)
> +    if (!optUUID && !optName && !optID)
>          optTable = true;
>  

The rest of the code looks good.

On Tue, Oct 20, 2020 at 12:27:27 +0200, Michal Privoznik wrote:
> Some completers for libvirt related tools might want to list
> domain IDs only. Just like the one I've implemented for
> virt-viewer [1]. I've worked around it using some awk magic,
> but if it was possible to just 'virsh list --id' then I could
> drop awk.

IMO completing numeric IDs doesn't make much sense, they aren't
descriptive at all and I don't really see a point for users using them
on a commandline. If you are expanding both names and IDs then you'll
have twice as much completion suggestions on an empty string.

Is there a case where it would actually make sense? Specifically in
virsh we almost always accept id/name/UUID interchangably for a
'domain'.

On 10/20/20 2:46 PM, Peter Krempa wrote:
> On Tue, Oct 20, 2020 at 12:27:27 +0200, Michal Privoznik wrote:
>> Some completers for libvirt related tools might want to list
>> domain IDs only. Just like the one I've implemented for
>> virt-viewer [1]. I've worked around it using some awk magic,
>> but if it was possible to just 'virsh list --id' then I could
>> drop awk.
> 
> IMO completing numeric IDs doesn't make much sense, they aren't
> descriptive at all and I don't really see a point for users using them
> on a commandline. If you are expanding both names and IDs then you'll
> have twice as much completion suggestions on an empty string.
> 
> Is there a case where it would actually make sense? Specifically in
> virsh we almost always accept id/name/UUID interchangably for a
> 'domain'.
> 

As I'm saying in the commit message - virt-viewer accepts --id, I don't 
expect ID completer to ever be implemented for virsh because as you say, 
domnames and/or uuids are accepted universally.

But adapting virt-viewer's completer is not going to be trivial anyway, 
so we might as well drop this :-)

Michal

On Tue, Oct 20, 2020 at 15:02:07 +0200, Michal Privoznik wrote:
> On 10/20/20 2:46 PM, Peter Krempa wrote:
> > On Tue, Oct 20, 2020 at 12:27:27 +0200, Michal Privoznik wrote:
> > > Some completers for libvirt related tools might want to list
> > > domain IDs only. Just like the one I've implemented for
> > > virt-viewer [1]. I've worked around it using some awk magic,
> > > but if it was possible to just 'virsh list --id' then I could
> > > drop awk.
> > 
> > IMO completing numeric IDs doesn't make much sense, they aren't
> > descriptive at all and I don't really see a point for users using them
> > on a commandline. If you are expanding both names and IDs then you'll
> > have twice as much completion suggestions on an empty string.
> > 
> > Is there a case where it would actually make sense? Specifically in
> > virsh we almost always accept id/name/UUID interchangably for a
> > 'domain'.
> > 
> 
> As I'm saying in the commit message - virt-viewer accepts --id, I don't
> expect ID completer to ever be implemented for virsh because as you say,
> domnames and/or uuids are accepted universally.

Okay, I've seen that you've mentioned the completer for virt-viewer but
I didn't realize it has an explicit --id. It makes sense though since VM
name allows numeric names, so if a completer suggests both names and ids
it might end up very confusing.

A potential alternative resolution to my other reply reviewing the code
would be to make 'virsh list --id' mutually exclusive with '--name' or
'--uuid'. That would make it usable with a completer completing IDs and
wouldn't hopefully give anyone false ideas of using it to fetch all 3
identifiers for completion.

On 10/20/20 3:14 PM, Peter Krempa wrote:
> On Tue, Oct 20, 2020 at 15:02:07 +0200, Michal Privoznik wrote:
>> On 10/20/20 2:46 PM, Peter Krempa wrote:
>>> On Tue, Oct 20, 2020 at 12:27:27 +0200, Michal Privoznik wrote:
>>>> Some completers for libvirt related tools might want to list
>>>> domain IDs only. Just like the one I've implemented for
>>>> virt-viewer [1]. I've worked around it using some awk magic,
>>>> but if it was possible to just 'virsh list --id' then I could
>>>> drop awk.
>>>
>>> IMO completing numeric IDs doesn't make much sense, they aren't
>>> descriptive at all and I don't really see a point for users using them
>>> on a commandline. If you are expanding both names and IDs then you'll
>>> have twice as much completion suggestions on an empty string.
>>>
>>> Is there a case where it would actually make sense? Specifically in
>>> virsh we almost always accept id/name/UUID interchangably for a
>>> 'domain'.
>>>
>>
>> As I'm saying in the commit message - virt-viewer accepts --id, I don't
>> expect ID completer to ever be implemented for virsh because as you say,
>> domnames and/or uuids are accepted universally.
> 
> Okay, I've seen that you've mentioned the completer for virt-viewer but
> I didn't realize it has an explicit --id. It makes sense though since VM
> name allows numeric names, so if a completer suggests both names and ids
> it might end up very confusing.

No, the --name switch is the default; so virt-viewer<TAB><TAB> will 
offer you only the names. Only if you type virt-viewer --id<TAB><TAB> 
you are presented with IDs and IDs only.

And of course mixing IDs with names is horrible thing to do (and I had 
that on mind when writing the virt-viewer completer), but so is UUID and 
name mixing. That is why in virsh we only run name completer and only 
for "domname" command we run UUID completer.

> 
> A potential alternative resolution to my other reply reviewing the code
> would be to make 'virsh list --id' mutually exclusive with '--name' or
> '--uuid'. That would make it usable with a completer completing IDs and
> wouldn't hopefully give anyone false ideas of using it to fetch all 3
> identifiers for completion.
> 

I'll think about it for v2. Thanks!

Michal