On 06/12/2012 06:59 AM, Peter Krempa wrote:
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.
Thanks; that helps.
@@ -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. */
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: list (all snapshots, no parent info needed) list --roots (only root snapshots) list --from (only direct children of --from) list --from --descendants (recursive children of --from) list --tree (all snapshots, parent info needed) list --tree --from (recursive children of --from, but also list from) Then the API that satisfy those combinations: 'new' API (aka 0.9.7-0.9.12) with --from (regardless of --descendants) -> use virDomainSnapshotNumChildren in its entirety new API without --from (regardless of -roots) -> use virDomainSnapshotNum in its entirety 'old' API (aka 0.8.0-0.9.6) without --from or --roots -> use virDomainSnapshotNum in its entirety old API without --from but with --roots -> use virDomainSnapshotNum, but then manually filter to just roots old with --from (regardless of --descendants) -> use virDomainSnapshotNum to get a global list, then manually filter to just the snapshot's children Additionally, if both --from and --tree are present, then we want --from to be included in the list. I really need to list this as a comment. I'll post a v2, adding comments and including your improvements.
+ /* fallback to old API */ + if (count < 0) { count = virDomainSnapshotNum(dom, flags);
More like: fall back to old API when doing children listing, or do initial probe when doing global listing. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org