Re: [PATCH 3/9] virsh: Implement VIR_DOMAIN_JOB_STATS_COMPLETED_KEEP for 'domjobinfo'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/25/19 9:01 AM, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
  tools/virsh-domain.c | 9 +++++++++
  tools/virsh.pod      | 7 ++++---
  2 files changed, 13 insertions(+), 3 deletions(-)


Renaming from 2/9 would ripple here, obviously.

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 99194c2f81..6e3814f1fd 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6025,6 +6025,10 @@ static const vshCmdOptDef opts_domjobinfo[] = {
       .type = VSH_OT_BOOL,
       .help = N_("return statistics of a recently completed job")
      },
+    {.name = "keep-completed",

In fact, you named the virsh command line option opposite from the flag name, and I like the CLI ordering better.

+     .type = VSH_OT_BOOL,
+     .help = N_("don't destroy statistics of a recently completed job when reading")
+    },

Should this flag imply --completed for convenience, or do you want to force the user to write --completed --keep-completed? The latter makes it possible to test that we catch incorrect use of the flag in isolation, but doesn't aid the command line user.

/me reads

Your implementation is the latter (the user has to type extra, rather than virsh letting one flag imply the other).

+++ b/tools/virsh.pod
@@ -1380,12 +1380,13 @@ Returns basic information about the domain.

  Abort the currently running domain job.

-=item B<domjobinfo> I<domain> [I<--completed>]
+=item B<domjobinfo> I<domain> [I<--completed>] [I<--keep-completed>]

Semantically, you could write this:

=item B<domjobinfo> I<domain> [I<--completed> [I<--keep-completed>]]

to show that the --keep-completed only makes sense with --completed. (Of course, that changes if you make one flag imply the other, in which case, the form you wrote is already best)

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux