[libvirt] [PATCH v2 02/10] virsh: Refactor the table output of the list command

[John Ferlan]

On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> Clean up some duplicate code so it is easier to add new parameters.  The
> tests need to be adjusted because after this patch there will be
> additional spaces at the end of lines in the output of virsh list, but
> this affects only the table output that is meant to be read by users.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  tests/virshtest.c            | 14 +++++++-------
>  tools/virsh-domain-monitor.c | 38 +++++++++++++++++++-------------------
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/virshtest.c b/tests/virshtest.c
> index 3fdae92b7834..5983b5b190d6 100644
> --- a/tests/virshtest.c
> +++ b/tests/virshtest.c
> @@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data ATTRIBUTE_UNUSED)
>  {
>    const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
>    const char *exp = "\
> - Id    Name                           State\n\
> -----------------------------------------------------\n\
> - 1     test                           running\n\
> + Id    Name                           State     \n\
> +-------------------------------------------------\n\
> + 1     test                           running   \n\
>  \n";
>    return testCompareOutputLit(exp, NULL, argv);
>  }
> @@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data ATTRIBUTE_UNUSED)
>  {
>    const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
>    const char *exp = "\
> - Id    Name                           State\n\
> -----------------------------------------------------\n\
> - 1     fv0                            running\n\
> - 2     fc4                            running\n\
> + Id    Name                           State     \n\
> +-------------------------------------------------\n\
> + 1     fv0                            running   \n\
> + 2     fc4                            running   \n\

[1]If only to make the output line up w/r/t \n...

>  \n";
>    return testCompareOutputLit(exp, NULL, argv);
>  }
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index db5bf4b2a566..24b902905968 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -1914,16 +1914,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
> 
>      /* print table header in legacy mode */
>      if (optTable) {
> +        vshPrintExtra(ctl, " %-5s %-30s %-10s", _("Id"), _("Name"), _("State"));
> +
>          if (optTitle)
> -            vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
> -                          _("Id"), _("Name"), _("State"), _("Title"),
> -                          "-----------------------------------------"
> -                          "-----------------------------------------");
> -        else
> -            vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
> -                          _("Id"), _("Name"), _("State"),
> -                          "-----------------------------------------"
> -                          "-----------");
> +            vshPrintExtra(ctl, " %-20s\n", _("Title"));

                                         ^^
I believe if Title were added there'd be an extra line once the
following dashes are printed (end w/ \n, start with \n)

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

It does seem in the expected virshtest.c output we're still off by 1.
That is there is 1 more "-" than extra space after the State column.

[1]
Instead of counting dashes...

    dashcount = 45;  /* Or some value - whatever it is */
...
    if (optTitle) {
...
        dashcount += 20;
   }
,,,
    for (i = 0; i < dashcount; i++)
        buf[i] = '-';
    vshPrintExtra(ctl, "%s\n", buf);
...

where of course buf is appropriately defined ;-)  A nit would be that
the old output was 3 dashes longer.


ACK with at least the fixup as a result of having Title on the line (and
it would be nice to add a test for it too)!

Whether you implement the '-' hack is up to you, but it may make future
adjustments more simple...

John

> +
> +        vshPrintExtra(ctl, "\n");
>      }
> 
>      for (i = 0; i < list->ndomains; i++) {
> @@ -1945,23 +1947,21 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>              state = -2;
> 
>          if (optTable) {
> +            vshPrint(ctl, " %-5s %-30s %-10s", id_buf,
> +                     virDomainGetName(dom),
> +                     state == -2 ? _("saved")
> +                     : virshDomainStateToString(state));
> +
>              if (optTitle) {
>                  if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
>                      goto cleanup;
> 
> -                vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
> -                         virDomainGetName(dom),
> -                         state == -2 ? _("saved")
> -                         : virshDomainStateToString(state),
> -                         title);
> +                vshPrint(ctl, " %-20s", title);
> 
>                  VIR_FREE(title);
> -            } else {
> -                vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
> -                         virDomainGetName(dom),
> -                         state == -2 ? _("saved")
> -                         : virshDomainStateToString(state));
>              }
> +
> +            vshPrint(ctl, "\n");
>          } else if (optUUID) {
>              if (virDomainGetUUIDString(dom, uuid) < 0) {
>                  vshError(ctl, "%s", _("Failed to get domain's UUID"));
>