[libvirt] [PATCH 3/8] backup: Introduce virDomainCheckpointPtr

[Eric Blake]

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