9:56 p.m.
On 06/26/2018 02:03 PM, Nir Soffer wrote:
On Wed, Jun 13, 2018 at 7:42 PM Eric Blake <eblake(a)redhat.com>
wrote:
> Prepare for introducing a bunch of new public APIs related to
> backup checkpoints by first introducing a new internal type
> and errors associated with that type. Checkpoints are modeled
> heavily after virDomainSnapshotPtr (both represent a point in
> time of the guest), although a snapshot exists with the intent
> of rolling back to that state, while a checkpoint exists to
> make it possible to create an incremental backup at a later
> time.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> include/libvirt/libvirt-domain-snapshot.h | 8 ++--
> include/libvirt/libvirt.h | 2 +
> include/libvirt/virterror.h | 5 ++-
> src/datatypes.c | 62
> ++++++++++++++++++++++++++++++-
> src/datatypes.h | 31 +++++++++++++++-
> src/libvirt_private.syms | 2 +
> src/util/virerror.c | 15 +++++++-
> 7 files changed, 118 insertions(+), 7 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain-snapshot.h
> b/include/libvirt/libvirt-domain-snapshot.h
> index e5a893a767..ff1e890cfc 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -31,15 +31,17 @@
> /**
> * virDomainSnapshot:
> *
> - * a virDomainSnapshot is a private structure representing a snapshot of
> - * a domain.
> + * A virDomainSnapshot is a private structure representing a snapshot of
> + * a domain. A snapshot captures the state of the domain at a point in
> + * time, with the intent that the guest can be reverted back to that
> + * state at a later time.
>
The extra context is very nice...
> */
> typedef struct _virDomainSnapshot virDomainSnapshot;
>
> /**
> * virDomainSnapshotPtr:
> *
> - * a virDomainSnapshotPtr is pointer to a virDomainSnapshot private
> structure,
> + * A virDomainSnapshotPtr is pointer to a virDomainSnapshot private
> structure,
> * and is the type used to reference a domain snapshot in the API.
> */
>
But I think users of this API would like to find it here, explaining the
public
type.
That's a pre-existing documentation issue (probably worth a separate
cleanup patch to a lot of files, if it really does render better to tie
the details to the 'Ptr' typedef rather than the opaque typedef).
> @@ -321,6 +322,8 @@ typedef enum {
> to guest-sync command
> (DEPRECATED)*/
> VIR_ERR_LIBSSH = 98, /* error in libssh transport
> driver */
> VIR_ERR_DEVICE_MISSING = 99, /* fail to find the desired
> device */
> + VIR_ERR_INVALID_DOMAIN_CHECKPOINT = 100,/* invalid domain checkpoint
> */
>
What is invalid checkpoint? It would be nice if there would not be
such thing.
Copied from the existing VIR_ERR_INVALID_DOMAIN_SNAPSHOT. Sadly, there
MUST be such a thing - it exists to (try and) catch bugs such as:
void *ptr = virAPI1() (which returns virDomainPtr)
virDomainCheckpointAPI2(ptr, ...) (which expects virDomainCheckpointPtr)
where you are passing in the wrong type, or such as:
virConnectPtr conn = virAPI1()
virDomainCheckpointPtr chk = virAPI2(conn)
virConnectClose(conn)
virDomainCheckpointAPI3(chk)
where you are passing in the right type but wrong order because the
checkpoint depends on a connection that you have closed
Also the comment does not add anything.
Such is the life of copy-and-paste. My excuse is that the code I copied
from has the same sort of poor comment.
> @@ -292,6 +293,21 @@ extern virClassPtr virAdmClientClass;
> } \
> } while (0)
>
> +# define virCheckDomainCheckpointReturn(obj, retval) \
> + do { \
> + virDomainCheckpointPtr _check = (obj); \
> + if (!virObjectIsClass(_check, virDomainCheckpointClass) || \
> + !virObjectIsClass(_check->domain, virDomainClass) || \
> + !virObjectIsClass(_check->domain->conn, virConnectClass)) { \
> + virReportErrorHelper(VIR_FROM_DOMAIN_CHECKPOINT, \
> + VIR_ERR_INVALID_DOMAIN_CHECKPOINT, \
> + __FILE__, __FUNCTION__, __LINE__, \
> + __FUNCTION__); \
>
I guess that this is invalid domain checkpoint. Isn't this a generic
error, providing a pointer of the wrong type?
Yes, except that libvirt already has the practice of distinguishing
error messages according to which type was expected.
> @@ -712,6 +739,8 @@ virStreamPtr virGetStream(virConnectPtr
conn);
> virNWFilterPtr virGetNWFilter(virConnectPtr conn,
> const char *name,
> const unsigned char *uuid);
> +virDomainCheckpointPtr virGetDomainCheckpoint(virDomainPtr domain,
> + const char *name);
>
I guess this implemented and documented elsewhere.
This is a function for internal use only; it is not exported as a public
function, but exists to mirror...
> virDomainSnapshotPtr virGetDomainSnapshot(virDomainPtr domain,
> const char *name);
...this existing snapshot function with the exact same amount of zero
comments.
It was implemented in this patch, in src/datatypes.c.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org