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 ebl...@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
