[libvirt] [PATCH 2/2] virsh: Implement new table API for virsh list

[Michal Privoznik]

On 08/10/2018 11:11 AM, Simon Kobyda wrote:
> Instead of printing it straight in virsh, it creates table struct
> which is filled with header and rows(domains). It allows us to know
> more about table before printing to calculate alignment right.
> 
> Signed-off-by: Simon Kobyda <skobyda at redhat.com>
> ---
>  tools/virsh-domain-monitor.c | 39 ++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b9b4f9739b..f6842877f5 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -39,6 +39,7 @@
>  #include "virmacaddr.h"
>  #include "virxml.h"
>  #include "virstring.h"
> +#include "vsh-table.h"
>  
>  VIR_ENUM_DECL(virshDomainIOError)
>  VIR_ENUM_IMPL(virshDomainIOError,
> @@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>      char id_buf[INT_BUFSIZE_BOUND(unsigned int)];
>      unsigned int id;
>      unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE;
> +    vshTablePtr table = NULL;
>  
>      /* construct filter flags */
>      if (vshCommandOptBool(cmd, "inactive") ||
> @@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>      /* print table header in legacy mode */
>      if (optTable) {
>          if (optTitle)
> -            vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
> -                          _("Id"), _("Name"), _("State"), _("Title"),
> -                          "-----------------------------------------"
> -                          "-----------------------------------------");
> +            table = vshTableNew("Id", "Name", "State", "Title", NULL);
>          else
> -            vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
> -                          _("Id"), _("Name"), _("State"),
> -                          "-----------------------------------------"
> -                          "-----------");
> +            table = vshTableNew("Id", "Name", "State", NULL);
> +
> +        if (!table)
> +            goto cleanup;
>      }
>  
>      for (i = 0; i < list->ndomains; i++) {
> @@ -1973,20 +1972,18 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
>              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);
> -
> +                vshTableRowAppend(table, id_buf, virDomainGetName(dom),
> +                                  state == -2 ? _("saved")
> +                                  : virshDomainStateToString(state),
> +                                  title, NULL);

You need to check for vshTableRowAppend() retval here.

>                  VIR_FREE(title);
>              } else {
> -                vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
> -                         virDomainGetName(dom),
> -                         state == -2 ? _("saved")
> -                         : virshDomainStateToString(state));
> +                vshTableRowAppend(table, id_buf, virDomainGetName(dom),
> +                                  state == -2 ? _("saved")
> +                                  : virshDomainStateToString(state),
> +                                  NULL);

Another approach might be to initialize title to NULL, and then set it
iff optTitle and then call:

   vshTableRowAppend(table, id_buf, virDomainGetName(dom),
                     state == -2 ? _("saved")
                     : virshDomainStateToString(state),
                     title, NULL);


Anyway, the rest looks good.

Michal