[libvirt] [PATCH 05/13] virsh: Implement vshTable API to snapshot-list.

[Michal Privoznik]

On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda <skobyda at redhat.com>
> ---
>  tools/virsh-snapshot.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index a4ea959230..5a02d2c786 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -41,6 +41,7 @@
>  #include "virstring.h"
>  #include "virxml.h"
>  #include "conf/snapshot_conf.h"
> +#include "vsh-table.h"
>  
>  /* Helper for snapshot-create and snapshot-create-as */
>  static bool
> @@ -1487,6 +1488,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
>      char *parent_snap = NULL;
>      virDomainSnapshotPtr start = NULL;
>      virshSnapshotListPtr snaplist = NULL;
> +    vshTablePtr table = NULL;
>  
>      VSH_EXCLUSIVE_OPTIONS_VAR(tree, name);
>      VSH_EXCLUSIVE_OPTIONS_VAR(parent, roots);
> @@ -1547,15 +1549,12 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
>  
>      if (!tree && !name) {
>          if (parent)
> -            vshPrintExtra(ctl, " %-20s %-25s %-15s %s",
> -                          _("Name"), _("Creation Time"), _("State"),
> -                          _("Parent"));
> +            table = vshTableNew("Name", "Creation Time", "State", "Parent", NULL);
>          else
> -            vshPrintExtra(ctl, " %-20s %-25s %s",
> -                          _("Name"), _("Creation Time"), _("State"));
> -        vshPrintExtra(ctl, "\n"
> -                           "------------------------------"
> -                           "------------------------------\n");
> +            table = vshTableNew("Name", "Creation Time", "State", NULL);
> +
> +        if (!table)
> +            goto cleanup;
>      }
>  
>      if (tree) {
> @@ -1614,13 +1613,20 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
>          strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z",
>                   &time_info);
>  
> -        if (parent)
> -            vshPrint(ctl, " %-20s %-25s %-15s %s\n",
> -                     snap_name, timestr, state, parent_snap);
> -        else
> -            vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state);
> +        if (parent) {
> +            if (vshTableRowAppend(table, snap_name, timestr, state, parent_snap,
> +                                  NULL) < 0)
> +                continue;
> +        } else {
> +            if (vshTableRowAppend(table, snap_name, timestr, state,
> +                                  NULL) < 0)
> +                continue;

What is the point of these 'continue'? Shouldn't we jump to cleanup instead?

> +        }
>      }
>  
> +    if (!tree && !name)

Or simply: if (table)

> +        vshTablePrintToStdout(table, ctl);
> +
>      ret = true;
>  
>   cleanup:
> @@ -1633,6 +1639,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
>      xmlFreeDoc(xml);
>      VIR_FREE(doc);
>      virshDomainFree(dom);
> +    vshTableFree(table);
>  
>      return ret;
>  }
> 

This is where I'm going to stop my review. You get the idea what changes
you need to do for v2.

Michal