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

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



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 redhat com>
>> ---
> [...]
>> 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


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