Re: [libvirt] [PATCH v4 05/20] backup: Introduce virDomainCheckpointPtr

On 2/6/19 2:18 PM, Eric Blake 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&gt; > > --- > v2: fix copy-and-paste issue in virerror.c [John] > --- > include/libvirt/virterror.h | 7 +++-- > src/util/virerror.c | 12 ++++++- > include/libvirt/libvirt.h | 2 ++ > src/datatypes.h | 31 ++++++++++++++++++- > src/datatypes.c | 62 ++++++++++++++++++++++++++++++++++++- > src/libvirt_private.syms | 2 ++ > 6 files changed, 111 insertions(+), 5 deletions(-) > Naturally something changed recently in datatypes.h... so you will have a merge conflict here.

> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > index 3c19ff5e2e..acbf03d0ea 100644 > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -4,7 +4,7 @@ > * Description: Provides the interfaces of the libvirt library to handle > * errors raised while using the library. > * > - * Copyright (C) 2006-2016 Red Hat, Inc. > + * Copyright (C) 2006-2019 Red Hat, Inc. I'll let someone else complain about your emacs macro ;-)

> --- a/include/libvirt/libvirt.h > +++ b/include/libvirt/libvirt.h > @@ -34,6 +34,8 @@ extern "C" { > # include <libvirt/libvirt-common.h> > # include <libvirt/libvirt-host.h> > # include <libvirt/libvirt-domain.h> > +typedef struct _virDomainCheckpoint virDomainCheckpoint; > +typedef virDomainCheckpoint *virDomainCheckpointPtr; This is a weird construct for this file... I think this should be in a libvirt-domain-checkpoint.h type file which I see is generated a few patches later and these lines moved. In the long run I guess as long as it's in a file it doesn't matter.

> +++ b/src/datatypes.h > @@ -1,7 +1,7 @@ > /* > * datatypes.h: management of structs for public data types > * > - * Copyright (C) 2006-2015 Red Hat, Inc. > + * Copyright (C) 2006-2018 Red Hat, Inc. haha 2018 - I know when you last touched this ;-)

> +/** > + * virGetDomainCheckpoint: > + * @domain: the domain to checkpoint > + * @name: pointer to the domain checkpoint name > + * > + * Allocates a new domain checkpoint object. When the object is no longer needed, > + * virObjectUnref() must be called in order to not leak data. > + * > + * Returns a pointer to the domain checkpoint object, or NULL on error. > + */ > +virDomainCheckpointPtr > +virGetDomainCheckpoint(virDomainPtr domain, const char *name) More recently we or I have been trying to enforce one line per argument for new code or changed methods.

> +{ > + virDomainCheckpointPtr ret = NULL; > + > + if (virDataTypesInitialize() < 0) > + return NULL; > + > + virCheckDomainGoto(domain, error); > + virCheckNonNullArgGoto(name, error); > + > + if (!(ret = virObjectNew(virDomainCheckpointClass))) > + goto error; Could return NULL too, but it is a copy of virGetDomainSnapshot Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt;