Re: [libvirt] [RFC PATCH 3/8] Snapshot API framework.

On Thu, Apr 01, 2010 at 06:07:22PM -0400, Chris Lalancette wrote: > Signed-off-by: Chris Lalancette <clalance(a)redhat.com&gt; > --- [...] > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > index 7cb483e..a8a97e3 100644 > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -1861,6 +1861,67 @@ int virDomainGetJobInfo(virDomainPtr dom, > virDomainJobInfoPtr info); > int virDomainAbortJob(virDomainPtr dom); > > +/** > + * virDomainSnapshot: > + * > + * a virDomainSnapshot is a private structure representing a snapshot of > + * a domain. > + */ > +typedef struct _virDomainSnapshot virDomainSnapshot; > + > +/** > + * virDomainSnapshotPtr: > + * > + * a virDomainSnapshotPtr is pointer to a virDomainSnapshot private structure, > + * and is the type used to reference a domain snapshot in the API. > + */ > +typedef virDomainSnapshot *virDomainSnapshotPtr; > + > +/* Take a snapshot of the current VM state */ > +virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, > + const char *xmlDesc, > + unsigned int flags); > + > +/* Dump the XML of a snapshot */ > +char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, > + unsigned int flags); > + > +/* Return the number of snapshots for this domain */ > +int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); > + > +/* Get the names of all snapshots for this domain */ > +int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, > + unsigned int flags); > + > +/* Get a handle to a named snapshot */ > +virDomainSnapshotPtr virDomainSnapshotLookupByName(virDomainPtr domain, > + const char *name, > + unsigned int flags); > + > +/* Check whether a domain has a snapshot which is currently used */ > +int virDomainHasCurrentSnapshot(virDomainPtr domain, unsigned flags); > + > +/* Get a handle to the current snapshot */ > +virDomainSnapshotPtr virDomainSnapshotCurrent(virDomainPtr domain, > + unsigned int flags); > + > +/* Set this snapshot as the current one for a domain, to be > + * used next time domain is started */ > +int virDomainCreateFromSnapshot(virDomainSnapshotPtr snapshot, > + unsigned int flags); > + > +/* Deactivate a snapshot */ > +typedef enum { > + VIR_DOMAIN_SNAPSHOT_DELETE_MERGE = (1 << 0), > + VIR_DOMAIN_SNAPSHOT_DELETE_MERGE_FORCE = (1 << 1), > + VIR_DOMAIN_SNAPSHOT_DELETE_DISCARD = (1 << 2), > + VIR_DOMAIN_SNAPSHOT_DELETE_DISCARD_FORCE = (1 << 3), > +} virDomainSnapshotDeleteFlags; Having a graphical reprentation of what the 4 options do on a snapshot arborescence would be nice but that's for documentation, in the meantime I would comment the enums. Also it seems to me the current set of options are expected to be mutually exclusive so why use bitfields ?

> +int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, > + unsigned int flags); > + > +int virDomainSnapshotFree(virDomainSnapshotPtr snapshot); > > /* A generic callback definition. Specific events usually have a customization > * with extra parameters */ [...] > diff --git a/src/libvirt.c b/src/libvirt.c > index 5247fe7..edb3084 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -683,6 +683,31 @@ virLibNWFilterError(virNWFilterPtr pool, virErrorNumber error, > } > > /** > + * virLibDomainSnapshotError: > + * @snapshot: the snapshot if available > + * @error: the error number > + * @info: extra information string > + * > + * Handle an error at the secret level Comment is obviously an old cut and paste :-)

> + */ > +static void > +virLibDomainSnapshotError(virDomainSnapshotPtr snapshot, virErrorNumber error, const char *info) > +{ > + virConnectPtr conn = NULL; > + const char *errmsg; > + > + if (error == VIR_ERR_OK) > + return; > + > + errmsg = virErrorMsg(error, info); > + if (error != VIR_ERR_INVALID_DOMAIN_SNAPSHOT) > + conn = snapshot->domain->conn; > + > + virRaiseError(conn, NULL, NULL, VIR_FROM_DOMAIN_SNAPSHOT, error, VIR_ERR_ERROR, > + errmsg, info, NULL, 0, 0, errmsg, info); > +} > + > +/** > * virRegisterNetworkDriver: > * @driver: pointer to a network driver block > * > @@ -12136,3 +12161,426 @@ error: > virDispatchError(conn); > return -1; [...] > +/** > + * virDomainSnapshotListNames: > + * @domain: a domain object > + * @names: array to collect the list of names of snapshots > + * @nameslen: size of @names > + * @flags: unused flag parameters; callers should pass 0 > + * > + * Collect the list of domain snapshots for the given domain, and store > + * their names in @names. > + * > + * Returns the number of domain snapshots found or -1 in case of error. > + */ Maybe need to indicate the strategy for freeing the names list.

[...] > +/** > + * virDomainCreateFromSnapshot wrong function name :-)

> + * @snapshot: a domain snapshot object > + * @flags: flag parameters > + * > + * Delete the snapshot. > + * > + * Returns 0 if the snapshot was successfully deleted, -1 on error. > + */ > +int > +virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, > + unsigned int flags) > +{ > + virConnectPtr conn; > + > + DEBUG("snapshot=%p, flags=%u", snapshot, flags); > + > + /* FIXME: make sure only one of the flags is set */ hence why use a bitfield allocation of the enums at all ?

> + virResetLastError(); > + > + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { > + virLibDomainSnapshotError(NULL, VIR_ERR_INVALID_DOMAIN_SNAPSHOT, __FUNCTION__); > + virDispatchError(NULL); > + return -1; > + } > + > + conn = snapshot->domain->conn; > + > + if (conn->driver->domainSnapshotDelete) { > + int ret = conn->driver->domainSnapshotDelete(snapshot, flags); > + if (ret < 0) > + goto error; > + return ret; > + } > + > + virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__); > +error: > + virDispatchError(conn); > + return -1; > +} > + > +/** > + * virDomainFree: function name error :-)

> + * @snapshot: a domain snapshot object > + * > + * Free the domain snapshot object. The snapshot itself is not modified. > + * The data structure is freed and should not be used thereafter. > + * > + * Returns 0 in case of success and -1 in case of failure. > + */ > +int > +virDomainSnapshotFree(virDomainSnapshotPtr snapshot) > +{ > + DEBUG("snapshot=%p", snapshot); > + > + virResetLastError(); > + > + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { > + virLibDomainSnapshotError(NULL, VIR_ERR_INVALID_DOMAIN_SNAPSHOT, __FUNCTION__); > + virDispatchError(NULL); > + return -1; > + } > + if (virUnrefDomainSnapshot(snapshot) < 0) { > + virDispatchError(NULL); > + return -1; > + } > + return 0; > +} Except for those few things this looks fine to me, ACK once fixed