[libvirt] [PATCH 0/2] virsh: Improve snapshot-list
Peter Krempa (2): virsh-snapshot: Add ability to print only snapshot names virsh-snapshot: Refactor virsh snapshot-list tools/virsh-snapshot.c | 143 ++++++++++++++++++++++++------------------------- tools/virsh.pod | 11 ++-- 2 files changed, 76 insertions(+), 78 deletions(-) -- 1.8.1.1
Help script creators by not having to parse the names from the table. --- tools/virsh-snapshot.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 11 +++++++---- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 3d82276..ed41014 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1556,6 +1556,12 @@ static const vshCmdOptDef opts_snapshot_list[] = { .flags = 0, .help = N_("with --from, list all descendants") }, + {.name = "name", + .type = VSH_OT_BOOL, + .flags = 0, + .help = N_("list snapshot names only") + }, + {.name = NULL} }; @@ -1578,10 +1584,17 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char timestr[100]; struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); + bool name = vshCommandOptBool(cmd, "name"); const char *from = NULL; virDomainSnapshotPtr start = NULL; vshSnapshotListPtr snaplist = NULL; + if (tree && name) { + vshError(ctl, "%s", + _("--tree and --name are mutually exclusive")); + return false; + } + dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) goto cleanup; @@ -1660,7 +1673,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) tree)) == NULL) goto cleanup; - if (!tree) { + if (!tree && !name) { if (show_parent) vshPrintExtra(ctl, " %-20s %-25s %-15s %s", _("Name"), _("Creation Time"), _("State"), @@ -1689,7 +1702,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } for (i = 0; i < snaplist->nsnaps; i++) { - const char *name; + const char *snap_name; /* free up memory from previous iterations of the loop */ VIR_FREE(parent); @@ -1699,8 +1712,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) VIR_FREE(doc); snapshot = snaplist->snaps[i].snap; - name = virDomainSnapshotGetName(snapshot); - assert(name); + snap_name = virDomainSnapshotGetName(snapshot); + assert(snap_name); + + if (name) { + vshPrint(ctl, "%s\n", snap_name); + continue; + } doc = virDomainSnapshotGetXMLDesc(snapshot, 0); if (!doc) @@ -1731,9 +1749,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent) vshPrint(ctl, " %-20s %-25s %-15s %s\n", - name, timestr, state, parent); + snap_name, timestr, state, parent); else - vshPrint(ctl, " %-20s %-25s %s\n", name, timestr, state); + vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state); } ret = true; diff --git a/tools/virsh.pod b/tools/virsh.pod index a5d8fe6..7fb89e4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2854,10 +2854,11 @@ accessible only from the original name. Output basic information about a named <snapshot>, or the current snapshot with I<--current>. -=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] +=item B<snapshot-list> I<domain> [I<--metadata>] [I<--no-metadata>] +[{I<--parent> | I<--roots> | [{I<--tree> | I<--name>}]}] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]] -[I<--metadata>] [I<--no-metadata>] [I<--leaves>] [I<--no-leaves>] -[I<--inactive>] [I<--active>] [I<--disk-only>] [I<--internal>] [I<--external>] +[I<--leaves>] [I<--no-leaves>] p[I<--inactive>] [I<--active>] +[I<--disk-only>] [I<--internal>] [I<--external>] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -2866,7 +2867,9 @@ If I<--parent> is specified, add a column to the output table giving the name of the parent of each snapshot. If I<--roots> is specified, the list will be filtered to just snapshots that have no parents. If I<--tree> is specified, the output will be in a tree format, listing -just snapshot names. These three options are mutually exclusive. +just snapshot names. These three options are mutually exclusive. If +I<--name> is specified only the snapshot name is printed. This option is +mutually exclusive with I<--tree>. If I<--from> is provided, filter the list to snapshots which are children of the given B<snapshot>; or if I<--current> is provided, -- 1.8.1.1
On 03/01/2013 08:43 AM, Peter Krempa wrote:
Help script creators by not having to parse the names from the table. --- tools/virsh-snapshot.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 11 +++++++---- 2 files changed, 31 insertions(+), 10 deletions(-)
Useful indeed. ACK. Borderline on whether this is 1.0.3 material - it doesn't affect libvirtd, but it's also arguably a new feature rather than bug fix. Probably best to be conservative and wait (so as to make the freeze mean something). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/01/13 19:25, Eric Blake wrote:
On 03/01/2013 08:43 AM, Peter Krempa wrote:
Help script creators by not having to parse the names from the table. --- tools/virsh-snapshot.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 11 +++++++---- 2 files changed, 31 insertions(+), 10 deletions(-)
Useful indeed. ACK. Borderline on whether this is 1.0.3 material - it doesn't affect libvirtd, but it's also arguably a new feature rather than bug fix. Probably best to be conservative and wait (so as to make the freeze mean something).
I will definitely wait after the release to push this. Thanks. Peter
On 03/01/13 19:25, Eric Blake wrote:
On 03/01/2013 08:43 AM, Peter Krempa wrote:
Help script creators by not having to parse the names from the table. --- tools/virsh-snapshot.c | 30 ++++++++++++++++++++++++------ tools/virsh.pod | 11 +++++++---- 2 files changed, 31 insertions(+), 10 deletions(-)
Useful indeed. ACK. Borderline on whether this is 1.0.3 material - it doesn't affect libvirtd, but it's also arguably a new feature rather than bug fix. Probably best to be conservative and wait (so as to make the freeze mean something).
Pushed now that the release is done. Thanks for the review. I will try making a more general version for 2/2. Peter
Simplify error handling and mutually exlusive option checking. --- Notes: I'm thinking of making the VSH_EXCLUSIVE_OPTION macro global in virsh. Mutually exclusive parameters are checked in many places throughout virsh and this might really simplify stuff sometimes. tools/virsh-snapshot.c | 125 ++++++++++++++++++++----------------------------- 1 file changed, 51 insertions(+), 74 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index ed41014..d3ee6c9 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1571,66 +1571,42 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; unsigned int flags = 0; - bool show_parent = false; int i; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; char *doc = NULL; virDomainSnapshotPtr snapshot = NULL; char *state = NULL; - char *parent = NULL; long long creation_longlong; time_t creation_time_t; char timestr[100]; struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); bool name = vshCommandOptBool(cmd, "name"); - const char *from = NULL; + bool from = vshCommandOptBool(cmd, "from"); + bool parent = vshCommandOptBool(cmd, "parent"); + bool roots = vshCommandOptBool(cmd, "roots"); + bool current = vshCommandOptBool(cmd, "current"); + const char *from_snap = NULL; + char *parent_snap = NULL; virDomainSnapshotPtr start = NULL; vshSnapshotListPtr snaplist = NULL; - if (tree && name) { - vshError(ctl, "%s", - _("--tree and --name are mutually exclusive")); - return false; +#define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2) \ + if (NAME1 && NAME2) { \ + vshError(ctl, _("Options --%s and --%s are mutually exclusive"), \ + #NAME1, #NAME2); \ + return false; \ } - dom = vshCommandOptDomain(ctl, cmd, NULL); - if (dom == NULL) - goto cleanup; - - if ((vshCommandOptBool(cmd, "from") || - vshCommandOptBool(cmd, "current")) && - vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from) < 0) - goto cleanup; + VSH_EXCLUSIVE_OPTIONS(tree, name); + VSH_EXCLUSIVE_OPTIONS(parent, roots); + VSH_EXCLUSIVE_OPTIONS(parent, tree); + VSH_EXCLUSIVE_OPTIONS(roots, tree); + VSH_EXCLUSIVE_OPTIONS(roots, from); + VSH_EXCLUSIVE_OPTIONS(roots, current); +#undef VSH_EXCLUSIVE_OPTIONS - if (vshCommandOptBool(cmd, "parent")) { - if (vshCommandOptBool(cmd, "roots")) { - vshError(ctl, "%s", - _("--parent and --roots are mutually exclusive")); - goto cleanup; - } - if (tree) { - vshError(ctl, "%s", - _("--parent and --tree are mutually exclusive")); - goto cleanup; - } - show_parent = true; - } else if (vshCommandOptBool(cmd, "roots")) { - if (tree) { - vshError(ctl, "%s", - _("--roots and --tree are mutually exclusive")); - goto cleanup; - } - if (from) { - vshError(ctl, "%s", - vshCommandOptBool(cmd, "current") ? - _("--roots and --current are mutually exclusive") : - _("--roots and --from are mutually exclusive")); - goto cleanup; - } - flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; - } #define FILTER(option, flag) \ do { \ if (vshCommandOptBool(cmd, option)) { \ @@ -1638,7 +1614,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) vshError(ctl, \ _("--%s and --tree are mutually exclusive"), \ option); \ - goto cleanup; \ + return false; \ } \ flags |= VIR_DOMAIN_SNAPSHOT_LIST_ ## flag; \ } \ @@ -1653,28 +1629,36 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) FILTER("external", EXTERNAL); #undef FILTER - if (vshCommandOptBool(cmd, "metadata")) { + if (roots) + flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + + if (vshCommandOptBool(cmd, "metadata")) flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; - } - if (vshCommandOptBool(cmd, "no-metadata")) { + + if (vshCommandOptBool(cmd, "no-metadata")) flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; - } if (vshCommandOptBool(cmd, "descendants")) { - if (!from) { + if (!from && !current) { vshError(ctl, "%s", _("--descendants requires either --from or --current")); - goto cleanup; + return false; } flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } - if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags, - tree)) == NULL) + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if ((from || current) && + vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from_snap) < 0) + goto cleanup; + + if (!(snaplist = vshSnapshotListCollect(ctl, dom, start, flags, tree))) goto cleanup; if (!tree && !name) { - if (show_parent) + if (parent) vshPrintExtra(ctl, " %-20s %-25s %-15s %s", _("Name"), _("Creation Time"), _("State"), _("Parent")); @@ -1682,12 +1666,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) vshPrintExtra(ctl, " %-20s %-25s %s", _("Name"), _("Creation Time"), _("State")); vshPrintExtra(ctl, "\n" -"------------------------------------------------------------\n"); - } - - if (!snaplist->nsnaps) { - ret = true; - goto cleanup; + "------------------------------" + "------------------------------\n"); } if (tree) { @@ -1705,7 +1685,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) const char *snap_name; /* free up memory from previous iterations of the loop */ - VIR_FREE(parent); + VIR_FREE(parent_snap); VIR_FREE(state); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); @@ -1716,25 +1696,24 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) assert(snap_name); if (name) { + /* just print the snapshot name */ vshPrint(ctl, "%s\n", snap_name); continue; } - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) + if (!(doc = virDomainSnapshotGetXMLDesc(snapshot, 0))) continue; - xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt); - if (!xml) + if (!(xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt))) continue; - if (show_parent) - parent = virXPathString("string(/domainsnapshot/parent/name)", - ctxt); + if (parent) + parent_snap = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); - state = virXPathString("string(/domainsnapshot/state)", ctxt); - if (state == NULL) + if (!(state = virXPathString("string(/domainsnapshot/state)", ctxt))) continue; + if (virXPathLongLong("string(/domainsnapshot/creationTime)", ctxt, &creation_longlong) < 0) continue; @@ -1749,7 +1728,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (parent) vshPrint(ctl, " %-20s %-25s %-15s %s\n", - snap_name, timestr, state, parent); + snap_name, timestr, state, parent_snap); else vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state); } @@ -1759,15 +1738,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) cleanup: /* this frees up memory from the last iteration of the loop */ vshSnapshotListFree(snaplist); - VIR_FREE(parent); + VIR_FREE(parent_snap); VIR_FREE(state); - if (start) - virDomainSnapshotFree(start); + virDomainSnapshotFree(start); xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); VIR_FREE(doc); - if (dom) - virDomainFree(dom); + virDomainFree(dom); return ret; } -- 1.8.1.1
On 03/01/2013 08:43 AM, Peter Krempa wrote:
Simplify error handling and mutually exlusive option checking.
s/exlusive/exclusive/
---
Notes: I'm thinking of making the VSH_EXCLUSIVE_OPTION macro global in virsh. Mutually exclusive parameters are checked in many places throughout virsh and this might really simplify stuff sometimes.
To make it more generic, you need to tweak it slightly.
+ bool from = vshCommandOptBool(cmd, "from"); + bool parent = vshCommandOptBool(cmd, "parent"); + bool roots = vshCommandOptBool(cmd, "roots"); + bool current = vshCommandOptBool(cmd, "current");
Here, you got lucky that all of the options you are checking can be represented as a variable name. But if we ever have a --two-part option that is mutually exclusive, then you need to distinguish between the option name and the variable name.
+#define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2) \
That is, I'd define this in terms of VSH_EXCLUSIVE_OPTIONS(COND1, COND2, NAME1, NAME2)
+ if (NAME1 && NAME2) { \
check COND1 and COND2 here
+ vshError(ctl, _("Options --%s and --%s are mutually exclusive"), \ + #NAME1, #NAME2); \
so that NAME1 and NAME2 can include dashes.
+ VSH_EXCLUSIVE_OPTIONS(tree, name); + VSH_EXCLUSIVE_OPTIONS(parent, roots); + VSH_EXCLUSIVE_OPTIONS(parent, tree); + VSH_EXCLUSIVE_OPTIONS(roots, tree); + VSH_EXCLUSIVE_OPTIONS(roots, from); + VSH_EXCLUSIVE_OPTIONS(roots, current); +#undef VSH_EXCLUSIVE_OPTIONS
But this part is slick :) Feels like a lot of churn in one patch; mixing variable renaming and logic flow changes made it a bit harder. But I'm not sure if it is any easier to split into multiple commits now. As written, it seems like this patch works, but I'm worried about getting the mutual exclusion check reusable. Definitely not worth the risk to 1.0.3; and maybe someone else wants to chime in on whether we need a v2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/01/13 19:38, Eric Blake wrote:
On 03/01/2013 08:43 AM, Peter Krempa wrote:
Simplify error handling and mutually exlusive option checking.
s/exlusive/exclusive/
---
Notes: I'm thinking of making the VSH_EXCLUSIVE_OPTION macro global in virsh. Mutually exclusive parameters are checked in many places throughout virsh and this might really simplify stuff sometimes.
To make it more generic, you need to tweak it slightly.
+ bool from = vshCommandOptBool(cmd, "from"); + bool parent = vshCommandOptBool(cmd, "parent"); + bool roots = vshCommandOptBool(cmd, "roots"); + bool current = vshCommandOptBool(cmd, "current");
Here, you got lucky that all of the options you are checking can be represented as a variable name. But if we ever have a --two-part option
I actually helped the luck a bit :)
that is mutually exclusive, then you need to distinguish between the option name and the variable name.
+#define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2) \
That is, I'd define this in terms of VSH_EXCLUSIVE_OPTIONS(COND1, COND2, NAME1, NAME2)
I'd go with two versions. One for the cool ones that actually match and the second one as you proposed here.
+ if (NAME1 && NAME2) { \
check COND1 and COND2 here
+ vshError(ctl, _("Options --%s and --%s are mutually exclusive"), \ + #NAME1, #NAME2); \
so that NAME1 and NAME2 can include dashes.
+ VSH_EXCLUSIVE_OPTIONS(tree, name); + VSH_EXCLUSIVE_OPTIONS(parent, roots); + VSH_EXCLUSIVE_OPTIONS(parent, tree); + VSH_EXCLUSIVE_OPTIONS(roots, tree); + VSH_EXCLUSIVE_OPTIONS(roots, from); + VSH_EXCLUSIVE_OPTIONS(roots, current); +#undef VSH_EXCLUSIVE_OPTIONS
But this part is slick :)
Feels like a lot of churn in one patch; mixing variable renaming and logic flow changes made it a bit harder. But I'm not sure if it is any easier to split into multiple commits now.
I will try to split it, when I'll be doing the more universal exclusive option checker.
As written, it seems like this patch works, but I'm worried about getting the mutual exclusion check reusable. Definitely not worth the risk to 1.0.3; and maybe someone else wants to chime in on whether we need a v2.
I will post a v2 ... I just wanted to see if this idea is reasonable. Peter
participants (2)
-
Eric Blake -
Peter Krempa