[libvirt] [PATCH v2 03/10] virsh: Add support for list -reason

[John Ferlan]

On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Add new parameter for the list command, --reason.  This adds another
> optional column in the table output of listed domains.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  tests/virshtest.c            | 16 ++++++++++++++++
>  tools/virsh-domain-monitor.c | 19 ++++++++++++++++++-
>  tools/virsh.pod              |  5 ++++-
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 5983b5b190d6..ecec898c7264 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -117,6 +117,18 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED)
>    return testCompareOutputLit(exp, NULL, argv);
>  }
> 
> +static int testCompareListReason(const void *data ATTRIBUTE_UNUSED)
> +{
> +    const char *const argv[] = { VIRSH_CUSTOM, "list", "--reason", NULL };
> +  const char *exp = "\
> + Id    Name                           State      Reason    \n\
> +------------------------------------------------------------\n\
> + 1     fv0                            running    unknown   \n\
> + 2     fc4                            running    unknown   \n\
> +\n";
> +  return testCompareOutputLit(exp, NULL, argv);
> +}
> +

Nice to have this test... Would be better to have one with Table too...

>  static int testCompareNodeinfoDefault(const void *data ATTRIBUTE_UNUSED)
>  {
>    const char *const argv[] = { VIRSH_DEFAULT, "nodeinfo", NULL };
> @@ -266,6 +278,10 @@ mymain(void)
>                      testCompareListCustom, NULL) != 0)
>          ret = -1;
> 
> +    if (virtTestRun("virsh list (with reason)",
> +                    testCompareListReason, NULL) != 0)
> +        ret = -1;
> +
>      if (virtTestRun("virsh nodeinfo (default)",
>                      testCompareNodeinfoDefault, NULL) != 0)
>          ret = -1;
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 24b902905968..e2ef2c566f84 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1848,6 +1848,10 @@ static const vshCmdOptDef opts_list[] = {
>       .type = VSH_OT_BOOL,
>       .help = N_("show domain title")
>      },
> +    {.name = "reason",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("show domain state reason")
> +    },
>      {.name = NULL}
>  };
> 
> @@ -1862,10 +1866,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>      bool optTable = vshCommandOptBool(cmd, "table");
>      bool optUUID = vshCommandOptBool(cmd, "uuid");
>      bool optName = vshCommandOptBool(cmd, "name");
> +    bool optReason = vshCommandOptBool(cmd, "reason");
>      size_t i;
>      char *title;
>      char uuid[VIR_UUID_STRING_BUFLEN];
>      int state;
> +    int state_reason;
>      bool ret = false;
>      virshDomainListPtr list = NULL;
>      virDomainPtr dom;
> @@ -1916,12 +1922,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>      if (optTable) {
>          vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State"));
> 
> +        if (optReason)
> +            vshPrintExtra(ctl, " %-10s", _("Reason"));
> +

Fairly easy to "dashcount += 10;"

or because the longest reason I can find is 19 characters, perhaps use
20... The only oddity is long state reason when title is also used. Your
call on this.

>          if (optTitle)
>              vshPrintExtra(ctl, " %-20s\n", _("Title"));
> 
>          vshPrintExtra(ctl, "\n%s",
>                        "-------------------------------------------------");
> 
> +        if (optReason)
> +            vshPrintExtra(ctl, "%s", "-----------");
> +

not needing this if use dashcount...

>          if (optTitle)
>              vshPrintExtra(ctl, "%s", "---------------------");
> 
> @@ -1936,7 +1948,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>          else
>              ignore_value(virStrcpyStatic(id_buf, "-"));
> 
> -        state = virshDomainState(ctl, dom, NULL);
> +        state = virshDomainState(ctl, dom, &state_reason);
> 
>          /* Domain could've been removed in the meantime */
>          if (state < 0)
> @@ -1952,6 +1964,11 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>                       state == -2 ? _("saved")
>                       : virshDomainStateToString(state));
> 
> +            if (optReason) {
> +                vshPrint(ctl, " %-10s",
> +                         virshDomainStateReasonToString(state, state_reason));
> +            }
> +
>              if (optTitle) {
>                  if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
>                      goto cleanup;
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 21ae4a3e5b46..7fddfc65d48c 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -393,7 +393,7 @@ value will generate output for the specific machine.
>  Inject NMI to the guest.
> 
>  =item B<list> [I<--inactive> | I<--all>]
> -              [I<--managed-save>] [I<--title>]
> +              [I<--managed-save>] [I<--title>] [I<--reason>]
>                { [I<--table>] | I<--name> | I<--uuid> }
>                [I<--persistent>] [I<--transient>]
>                [I<--with-managed-save>] [I<--without-managed-save>]
> @@ -531,6 +531,9 @@ If I<--title> is specified, then the short domain description (title) is
>  printed in an extra column. This flag is usable only with the default
>  I<--table> output.
> 
> +If I<--reason> is specified, then the state reason is printed in an extra
> +column. This flag is usable only with the default I<--table> output.
> +

This should go after the Example since it seems that's related to the
--title option...

Might be nice to explain what a "state reason" is.... Maybe even provide
another example with a couple of different state reasons for different
States (eg, runnning, migrating, paused, etc states)


ACK w/ a few adjustments.  Your call on dashcount - I just thought it'd
be easier.


John

>  Example:
> 
>  B<virsh> list --title
>