Re: [PATCH v8 07/21] backup: Allow for lists of checkpoint objects

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Apr 17, 2019 at 09:09:07 -0500, Eric Blake wrote:
> Create a new file for managing a list of checkpoint objects, borrowing
> heavily from existing virDomainSnapshotObjList paradigms.
> 
> Note that while checkpoints definitely have a use case for multiple
> children to a single parent (create a base snapshot, create a child
> snapshot, revert to the base, then create another child snapshot),
> it's harder to predict how checkpoints will play out with reverting to
> prior points in time. Thus, in initial use, we may find that in a list
> of checkpoints, you never have more than one child.  However, as the
> snapshot machinery is already generic, it is easier to reuse the
> generic machinery that tracks relations between domain moments than it
> is to open-code a new list-management scheme just for checkpoints.
> 
> The virDomainMomentObjList code is not quite polymorphic enough until
> we patch virDomainMomentDef to be a proper virObject, but doing that
> is a bigger audit.  So for now, I had to duplicate the cleanup calls
> based on a bool flag for which type needs cleaning. Oh well.

I'm afraid that with this being committed the motivation to refactor it
properly will be gone.

> 
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
>  src/conf/checkpoint_conf.h            |   7 +
>  src/conf/domain_conf.h                |   2 +
>  src/conf/virconftypes.h               |   6 +
>  src/conf/virdomaincheckpointobjlist.h |  74 +++++++++
>  src/conf/virdomainmomentobjlist.h     |   2 +-
>  src/conf/virdomainobjlist.h           |   7 +-
>  src/conf/Makefile.inc.am              |   2 +
>  src/conf/checkpoint_conf.c            |  86 ++++++++++
>  src/conf/domain_conf.c                |   6 +
>  src/conf/virdomaincheckpointobjlist.c | 223 ++++++++++++++++++++++++++
>  src/conf/virdomainmomentobjlist.c     |  40 ++++-
>  src/conf/virdomainobjlist.c           |  11 ++
>  src/conf/virdomainsnapshotobjlist.c   |   2 +-
>  src/libvirt_private.syms              |  18 +++
>  14 files changed, 477 insertions(+), 9 deletions(-)
>  create mode 100644 src/conf/virdomaincheckpointobjlist.h
>  create mode 100644 src/conf/virdomaincheckpointobjlist.c

[...]

> diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
> index 87f76e6518..cb0d71936f 100644
> --- a/src/conf/checkpoint_conf.c
> +++ b/src/conf/checkpoint_conf.c
> @@ -35,6 +35,7 @@
>  #include "virerror.h"
>  #include "virxml.h"
>  #include "virstring.h"
> +#include "virdomaincheckpointobjlist.h"
> 
>  #define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
> 
> @@ -548,3 +549,88 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,
> 
>      return virBufferContentAndReset(&buf);
>  }
> +
> +
> +int
> +virDomainCheckpointRedefinePrep(virDomainPtr domain,
> +                                virDomainObjPtr vm,
> +                                virDomainCheckpointDefPtr *defptr,
> +                                virDomainMomentObjPtr *chk,
> +                                virDomainXMLOptionPtr xmlopt,
> +                                bool *update_current)
> +{
> +    virDomainCheckpointDefPtr def = *defptr;
> +    char uuidstr[VIR_UUID_STRING_BUFLEN];
> +    virDomainMomentObjPtr other;
> +    virDomainCheckpointDefPtr otherdef;
> +
> +    virUUIDFormat(domain->uuid, uuidstr);
> +
> +    /* TODO: Move this and snapshot version into virDomainMoment */
> +    /* Prevent circular chains */
> +    if (def->common.parent) {
> +        if (STREQ(def->common.name, def->common.parent)) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("cannot set checkpoint %s as its own parent"),
> +                           def->common.name);
> +            return -1;
> +        }
> +        other = virDomainCheckpointFindByName(vm->checkpoints, def->common.parent);
> +        if (!other) {
> +            virReportError(VIR_ERR_INVALID_ARG,
> +                           _("parent %s for checkpoint %s not found"),
> +                           def->common.parent, def->common.name);
> +            return -1;
> +        }
> +        otherdef = virDomainCheckpointObjGetDef(other);
> +        while (otherdef->common.parent) {
> +            if (STREQ(otherdef->common.parent, def->common.name)) {
> +                virReportError(VIR_ERR_INVALID_ARG,
> +                               _("parent %s would create cycle to %s"),
> +                               otherdef->common.name, def->common.name);
> +                return -1;
> +            }
> +            other = virDomainCheckpointFindByName(vm->checkpoints,
> +                                                  otherdef->common.parent);
> +            if (!other) {
> +                VIR_WARN("checkpoints are inconsistent for %s",
> +                         vm->def->name);
> +                break;
> +            }
> +            otherdef = virDomainCheckpointObjGetDef(other);
> +        }
> +    }
> +
> +    if (!def->common.dom ||
> +        memcmp(def->common.dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("definition for checkpoint %s must use uuid %s"),
> +                       def->common.name, uuidstr);
> +        return -1;
> +    }
> +    if (virDomainCheckpointAlignDisks(def) < 0)
> +        return -1;
> +
> +    other = virDomainCheckpointFindByName(vm->checkpoints, def->common.name);
> +    otherdef = other ? virDomainCheckpointObjGetDef(other) : NULL;
> +    if (other) {
> +        if (!virDomainDefCheckABIStability(otherdef->common.dom,
> +                                           def->common.dom, xmlopt))
> +            return -1;
> +
> +        if (other == virDomainCheckpointGetCurrent(vm->checkpoints)) {
> +            *update_current = true;
> +            virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> +        }
> +
> +        /* Drop and rebuild the parent relationship, but keep all
> +         * child relations by reusing chk.  */
> +        virDomainMomentDropParent(other);
> +        virDomainCheckpointDefFree(otherdef);
> +        other->def = &(*defptr)->common;
> +        *defptr = NULL;
> +        *chk = other;
> +    }
> +
> +    return 0;
> +}

[...]

> diff --git a/src/conf/virdomaincheckpointobjlist.c b/src/conf/virdomaincheckpointobjlist.c
> new file mode 100644
> index 0000000000..dbc1c25c5e
> --- /dev/null
> +++ b/src/conf/virdomaincheckpointobjlist.c
> @@ -0,0 +1,223 @@

[...]

> +
> +#define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
> +
> +VIR_LOG_INIT("conf.virdomaincheckpointobjlist");
> +
> +struct _virDomainCheckpointObjList {
> +    virDomainMomentObjListPtr base;
> +};

Is this just for compile time typechecking?

> +virDomainMomentObjPtr
> +virDomainCheckpointAssignDef(virDomainCheckpointObjListPtr checkpoints,
> +                             virDomainCheckpointDefPtr def)
> +{
> +    return virDomainMomentAssignDef(checkpoints->base, &def->common);
> +}
> +
> +
> +static bool
> +virDomainCheckpointFilter(virDomainMomentObjPtr obj ATTRIBUTE_UNUSED,
> +                          unsigned int flags)
> +{
> +    /* For now, we have no further filters than what the common code handles. */
> +    virCheckFlags(0, false);
> +    return true;
> +}
> +
> +
> +virDomainCheckpointObjListPtr
> +virDomainCheckpointObjListNew(void)
> +{
> +    virDomainCheckpointObjListPtr checkpoints;
> +
> +    if (VIR_ALLOC(checkpoints) < 0)
> +        return NULL;
> +    checkpoints->base = virDomainMomentObjListNew(false);
> +    if (!checkpoints->base) {
> +        VIR_FREE(checkpoints);
> +        return NULL;
> +    }
> +    return checkpoints;
> +}
> +
> +
> +void
> +virDomainCheckpointObjListFree(virDomainCheckpointObjListPtr checkpoints)
> +{
> +    if (!checkpoints)
> +        return;
> +    virDomainMomentObjListFree(checkpoints->base);
> +    VIR_FREE(checkpoints);
> +}
> +
> +
> +static int
> +virDomainCheckpointObjListGetNames(virDomainCheckpointObjListPtr checkpoints,
> +                                   virDomainMomentObjPtr from,
> +                                   char **const names,
> +                                   int maxnames,
> +                                   unsigned int flags)
> +{
> +    /* We intentionally chose our public flags to match the common flags */
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_ROOTS ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_ROOTS);
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_TOPOLOGICAL ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL);
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_LEAVES ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_LEAVES);
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_NO_LEAVES);
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_METADATA ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_METADATA);
> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA ==
> +           (int) VIR_DOMAIN_MOMENT_LIST_NO_METADATA);

This looks like it should be near the VIR_DOMAIN_CHECKPOINT_LIST enum
rather than somewhere in the code randomly.

> +
> +    return virDomainMomentObjListGetNames(checkpoints->base, from, names,
> +                                          maxnames, flags,
> +                                          virDomainCheckpointFilter, 0);
> +}

[...]

> diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
> index 7f90632a53..3dd6da7acb 100644
> --- a/src/conf/virdomainmomentobjlist.c
> +++ b/src/conf/virdomainmomentobjlist.c

[...]

> @@ -242,23 +258,35 @@ virDomainMomentAssignDef(virDomainMomentObjListPtr moments,
> 
> 
>  static void
> -virDomainMomentObjListDataFree(void *payload,
> -                               const void *name ATTRIBUTE_UNUSED)
> +virDomainCheckpointObjListDataFree(void *payload,
> +                                   const void *name ATTRIBUTE_UNUSED)
>  {
>      virDomainMomentObjPtr obj = payload;
> 
> -    virDomainMomentObjFree(obj);
> +    virDomainCheckpointObjFree(obj);
> +}
> +
> +
> +static void
> +virDomainSnapshotObjListDataFree(void *payload,
> +                                 const void *name ATTRIBUTE_UNUSED)
> +{
> +    virDomainMomentObjPtr obj = payload;
> +
> +    virDomainSnapshotObjFree(obj);
>  }
> 
> 
>  virDomainMomentObjListPtr
> -virDomainMomentObjListNew(void)
> +virDomainMomentObjListNew(bool snapshot)

Please create a new entrypoint for this. I don't see the need to
overload it using the argument.

>  {
>      virDomainMomentObjListPtr moments;
> 
>      if (VIR_ALLOC(moments) < 0)
>          return NULL;
> -    moments->objs = virHashCreate(50, virDomainMomentObjListDataFree);
> +    moments->objs = virHashCreate(50,
> +                                  snapshot ? virDomainSnapshotObjListDataFree :
> +                                  virDomainCheckpointObjListDataFree);
>      if (!moments->objs) {
>          VIR_FREE(moments);
>          return NULL;

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux