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