On Wed, Mar 24, 2010 at 09:52:51AM +0100, Paolo Bonzini wrote:
> >> > /* Return the number of snapshots for this domain */
> >> > int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags);
> >>
> >> Shouldn't this be called virDomainNumOfSnapshots to be consistent with
> >> the naming or other num-of functions?
> >
> >Gah. I struggled with this; I like the fact that all of the rest of them
> >start with virDomainSnapshot; this one doesn't fit the mold. I don't care
> >too much either way.
>
> virDomain*Snapshots for APIs that take a virDomainPtr seems consistent too.
>
> >> > /* Deactivate a snapshot - with no flags, the snapshot is not used
> >anymore,
> >> > * but also not removed. With a MERGE flag, it merges the snapshot
> >into
> >> > * the base image. With a DISCARD flag, it deletes the snapshot.
> >MERGE
> >> > * and DISCARD are mutually-exclusive. Note that this operation can
> >> > * generally only happen when the domain is shut down, though this is
> >> > * hypervisor-specific */
> >> > typedef enum {
> >> > VIR_DOMAIN_SNAPSHOT_DEACTIVATE_MERGE,
> >> > VIR_DOMAIN_SNAPSHOT_DEACTIVATE_DISCARD,
> >> > } virDomainSnapshotDeactivate;
> >> > int virDomainSnapshotDeactivate(virDomainSnapshotPtr snapshot,
> >> > unsigned int flags);
> >>
> >> I'm not sure if virDomainSnapshotDeactivate is a good name.
> >
> >I agree it's not a great name. I didn't like Dan's original
> >proposal of "virDomainSnapshotDelete", though, since it doesn't
> >exactly seem to fit the situation either. Any more suggestions for
> >a name?
>
> Several things are not clear about this API to me too. :-)
>
> 1) Would discarding also discard all the snapshots depending on the
> discarded one. If so, what about a --force flag that you need to pass
> if the delete operation would delete other snapshots recursively?
Yes, if there were children of this snapshot they would be killed.
Agreed, that we could use a flag to allow that behaviour to be controlled,
either refusing to discard children, or forcing discard.
> 2) Likewise, merging would invalidate all the other snapshots depending
> on the base image (and discard them). The same --force option then
> could apply as well.
It would only discard snapshots depending on the snapshot being merge
onto, eg in
+-> F
|
A -> B -> C -> D
|
+-> G
If you merged D onto C, then A, B, F & G would all still be valid:
+-> F
|
A -> B -> C
|
+-> G
If you merged C onto B, then F & G would be invalid, but D would
still be valid
A -> B ------> D
> However, I think there is a fundamental difference between the two
> cases; only one of them modifies the base image I'm not sure that
> something that modifies the base image rightfully belongs into the
> virDomainSnapshot* namespace. So my proposal would be
>
> /* Start using the base image again. If snapshot != NULL, merge
> the given snapshot in the base image again. */
> virDomainDisableSnapshots(virDomainPtr domain,
> virDomainSnapshotPtr snapshot, int flags);
>
> /* Discard the given snapshot and, if --force is given, all that
> depend on it. Fail if it is active. */
> virDomainSnapshotDiscard(virDomainSnapshotPtr snapshot, int flags);
I don't see any need to special case the base image here. The main
virDOmainSnapshotDelete() API already lets you discard all snapshots
until you get to the base image - VMWare/VirtualBox already demonstrate
this is sufficient.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list