On 04/02/2010 12:17 PM, Daniel Veillard wrote:
> On Thu, Apr 01, 2010 at 06:07:22PM -0400, Chris Lalancette wrote:
>> Signed-off-by: Chris Lalancette <clalance@xxxxxxxxxx>
>> ---
> [...]
>> 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 ?
Actually, this is the one area where the new set of patches are changing
semantics. All of these flags are going to be removed except for just
one, which is "DISCARD_CHILDREN". I'll add documentation for that.
>
>> +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 :-)
Oops, yeah, fixed now.
>
>> + */
>> +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.
Added now.
>
>
> [...]
>> +/**
>> + * virDomainCreateFromSnapshot
>
> wrong function name :-)
Yeah, I noticed that after posting the series. Fixed now.
>
>> + * @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 ?
Yeah, this was silliness. It's all gone now since we only have one flag.
>
>> + 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 :-)
Fixed.
>
>> + * @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
Great, thanks.
--
Chris Lalancette
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list