9:43 a.m.
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
9:43 a.m.
New subject: [libvirt] [PATCH 1/2] virsh-snapshot: Add ability to print only snapshot names
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
12:25 p.m.
New subject: [libvirt] [PATCH 1/2] virsh-snapshot: Add ability to print only snapshot names
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
4:13 p.m.
New subject: [libvirt] [PATCH 1/2] virsh-snapshot: Add ability to print only snapshot names
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
2:15 a.m.
New subject: [libvirt] [PATCH 1/2] virsh-snapshot: Add ability to print only snapshot names
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
9:43 a.m.
New subject: [libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list
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
12:38 p.m.
New subject: [libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list
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
4:20 p.m.
New subject: [libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list
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
4283
days inactive
4287
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa