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

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 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 ?

> +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

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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