[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/6] snapshot: new virsh function factored from snapshot-list



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]