On 09/18/2018 04:21 PM, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda <skobyda@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
