On 06/26/2018 02:03 PM, Nir Soffer wrote:
On Wed, Jun 13, 2018 at 7:42 PM Eric Blake <eblake@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@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