[libvirt] [PATCH 3/8] backup: Introduce virDomainCheckpointPtr
Eric Blake
eblake at redhat.com
Wed Jun 27 02:56:25 UTC 2018
On 06/26/2018 02:03 PM, Nir Soffer wrote:
> On Wed, Jun 13, 2018 at 7:42 PM Eric Blake <eblake at 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 at 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
More information about the libvir-list
mailing list