On 06/12/12 14:54, Peter Krempa wrote:
On 06/09/12 06:34, Eric Blake wrote:
This patch copies just the fallback code out of cmdSnapshotList,
and keeps the snapshot objects around rather than just their
name for easier manipulation. It looks forward to a future
patch when we add a way to list all snapshot objects at once,
and the next patch will simplify cmdSnapshotList to take
advantage of this factorization.
I know it's copied code, but this error message doesn't look helpful. I think it's worth improving: "Failed to collect snapshot list" perhaps?
+ goto cleanup;
+ }
I had a very hard time parsing the code flow in this block, I'd rewrite this block as follows:
I forgot to attach the patch for the rewrite.
Peter
diff --git a/tools/virsh.c b/tools/virsh.c
index 488f8a4..c3aa92f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -16627,7 +16627,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));
@@ -16641,36 +16641,40 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom,
/* This is the interface available in 0.9.12 and earlier,
* including fallbacks to 0.9.4 and earlier. */
if (from) {
- fromname = virDomainSnapshotGetName(from);
- if (!fromname) {
+ if (!(fromname = virDomainSnapshotGetName(from))) {
vshError(ctl, "%s", _("Could not get snapshot name"));
goto cleanup;
}
+
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++;
+
+ /* try new API at first */
+ if (!ctl->useSnapshotOld &&
+ (count = virDomainSnapshotNumChildren(from, flags)) < 0 &&
+ last_error && last_error->code == VIR_ERR_NO_SUPPORT) {
+ /* We can emulate --from. */
+ /* XXX can we also emulate --leaves? */
+ virFreeError(last_error);
+ lastError = NULL;
+ ctl->useSnapshotOld = true;
+ flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
}
- } else {
+
+ if (tree && count >= 0)
+ count++;
+ }
+
+ /* fallback to old API */
+ 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;
@@ -16682,7 +16686,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;
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list