4:40 a.m.
On Thu, Dec 10, 2015 at 03:15:43PM -0500, John Ferlan wrote:
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(a)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...
You mean Title, not Table, right? This is Table output (default). With
Title I would find out that there is extra newline, I'll include that as well.
> 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
>