7:42 p.m.
On 06/12/2012 05:27 PM, Eric Blake wrote:
I'm off on my versioning - this was 0.9.6 and earlier.
> if (from) {
There's multiple things going on here:
First, the valid combinations:
Then the API that satisfy those combinations:
I really need to list this as a comment. I'll post a v2, adding
comments and including your improvements.
Here's what I'll squash for my v2:
diff --git i/tools/virsh.c w/tools/virsh.c
index 768d189..537496f 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -16848,7 +16848,7 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
{
int i;
char **names = NULL;
- int count = 0;
+ int count = -1;
bool descendants = false;
bool roots = false;
vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist));
@@ -16859,8 +16859,24 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
/* 0.9.13 will be adding a new listing API. */
- /* This is the interface available in 0.9.12 and earlier,
- * including fallbacks to 0.9.4 and earlier. */
+ /* This uses the interfaces available in 0.8.0-0.9.6
+ * (virDomainSnapshotListNames, global list only) and in
+ * 0.9.7-0.9.12 (addition of virDomainSnapshotListChildrenNames
+ * for child listing, and new flags), as follows, with [*] by the
+ * combinations that need parent info (either for filtering
+ * purposes or for the resulting tree listing):
+ * old new
+ * list global as-is global as-is
+ * list --roots *global + filter global + flags
+ * list --from *global + filter child + flags
+ * list --from --descendants *global + filter child + flags
+ * list --tree *global as-is *global as-is
+ * list --tree --from *global + filter *child + flags
+ *
+ * Additionally, when --tree and --from are both used, from is
+ * added to the final list as the only element without a parent.
+ * Otherwise, --from does not appear in the final list.
+ */
if (from) {
fromname = virDomainSnapshotGetName(from);
if (!fromname) {
@@ -16870,28 +16886,30 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0;
if (tree)
flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
- count = ctl->useSnapshotOld ? -1 :
- virDomainSnapshotNumChildren(from, flags);
- if (count < 0) {
- if (ctl->useSnapshotOld ||
- last_error->code == VIR_ERR_NO_SUPPORT) {
- /* We can emulate --from. */
- /* XXX can we also emulate --leaves? */
- virFreeError(last_error);
- last_error = NULL;
- ctl->useSnapshotOld = true;
- flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
- count = virDomainSnapshotNum(dom, flags);
- }
- } else if (tree) {
- count++;
+
+ /* First, determine if we can use the new child listing API. */
+ if (!ctl->useSnapshotOld &&
+ (count = virDomainSnapshotNumChildren(from, flags) < 0) &&
+ last_error->code == VIR_ERR_NO_SUPPORT) {
+ /* We can emulate --from. */
+ /* XXX can we also emulate --leaves? */
+ virFreeError(last_error);
+ last_error = NULL;
+ ctl->useSnapshotOld = true;
+ flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
}
- } else {
+ if (tree && count >= 0)
+ count++;
+ }
+
+ /* Global listing (including fallback when --from failed with
+ * child listing). */
+ if (count < 0) {
count = virDomainSnapshotNum(dom, flags);
/* Fall back to simulation if --roots was unsupported. */
/* XXX can we also emulate --leaves? */
- if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG
&&
+ if (!from && count < 0 && last_error->code ==
VIR_ERR_INVALID_ARG &&
(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
virFreeError(last_error);
last_error = NULL;
@@ -16903,7 +16921,7 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
if (count < 0) {
if (!last_error)
- vshError(ctl, _("missing support"));
+ vshError(ctl, _("failed to collect snapshot list"));
goto cleanup;
}
@@ -16912,6 +16930,7 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
names = vshCalloc(ctl, sizeof(*names), count);
+ /* Now that we have a count, collect the list. */
if (from && !ctl->useSnapshotOld) {
if (tree) {
if (count)
@@ -16940,6 +16959,10 @@ vshSnapshotListCollect(vshControl *ctl,
virDomainPtr dom,
goto cleanup;
}
+ /* Collect parents when needed. With the new API, --tree and
+ * --from together put from as the first element without a parent;
+ * with the old API we still need to do a post-process filtering
+ * based on all parent information. */
if (tree || (from && ctl->useSnapshotOld)) {
for (i = (from && !ctl->useSnapshotOld); i < count; i++) {
if (from && ctl->useSnapshotOld && STREQ(names[i],
fromname)) {
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org