[libvirt] [PATCH 06/16] snapshot: Track current snapshot in virDomainSnapshotObjList

[John Ferlan]

On 3/20/19 1:40 AM, Eric Blake wrote:
> It is easier to track the current snapshot as part of the list of
> snapshots. In particular, doing so lets us guarantee that the current
> snapshot is cleared if that snapshot is removed from the list (rather
> than depending on the caller to do so, and risking a use-after-free
> problem).  This requires the addition of several new accessor
> functions.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/domain_conf.h              |  1 -
>  src/conf/virdomainsnapshotobjlist.h | 10 +++--
>  src/conf/snapshot_conf.c            |  4 +-
>  src/conf/virdomainsnapshotobjlist.c | 63 ++++++++++++++++++++++++-----
>  src/libvirt_private.syms            |  4 ++
>  src/qemu/qemu_domain.c              | 14 +++----
>  src/qemu/qemu_driver.c              | 47 +++++++++------------
>  src/test/test_driver.c              | 38 ++++++++---------
>  8 files changed, 109 insertions(+), 72 deletions(-)
> 

[...]

> diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
> index be44bdde71..1eecb89a5d 100644
> --- a/src/conf/virdomainsnapshotobjlist.c
> +++ b/src/conf/virdomainsnapshotobjlist.c

[...]

> +
> +
> +/* Remove snapshot from the list; return true if it was current */
> +bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
>                                      virDomainSnapshotObjPtr snapshot)

NIT: One of these things is not like the others ;-)

bool
virDomain...

>  {
> +    bool ret = snapshots->current == snapshot;
>      virHashRemoveEntry(snapshots->objs, snapshot->def->name);
> +    if (ret)
> +        snapshots->current = NULL;

Slick, this is how testDomainSnapshotDiscardAll can alter it's logic.
Took me until the end of the patch to find ;-)... and coverity didn't
whine about one function checking return while the others don't.

> +    return ret;
>  }
> 

[...]

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7e0e76a31a..6c71382b93 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>          if (snap == NULL) {
>              virDomainSnapshotDefFree(def);
>          } else if (cur) {
> +            if (current)
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Too many snapshots claiming to be current for domain %s"),
> +                               vm->def->name);

Even though we generate this message we go ahead and update @current to
@snap. Should this be an "if (current) ... else ... " ?

Additionally if someone's really AFU'd, they could get more than one
message; whereas, previously they'd only get one such message.

>              current = snap;
> -            if (!vm->current_snapshot)
> -                vm->current_snapshot = snap;
>          }
> 
>          VIR_FREE(fullpath);
> @@ -495,13 +497,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>                         _("Failed to fully read directory %s"),
>                         snapDir);
> 
> -    if (vm->current_snapshot != current) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Too many snapshots claiming to be current for domain %s"),
> -                       vm->def->name);
> -        vm->current_snapshot = NULL;

Previously if this happened, then current was set to NULL, but the
following will set it to the last snap declared to be current.

Is that expected?  If not, then perhaps the if (current) above needs to
add a "current = NULL;" along with the error message. Of course that
leads to the possibility of others declaring themselves current and
possibly having multiple errors splatted. Only seems to matter for
someone running debug or looking at debug logs since we don't fail.

BTW: This is one of those current gray areas of making two changes in
one patch.  One change being the usage of the accessors and the other
being the alteration of when this message gets splatted.

> -    }
> -
> +    virDomainSnapshotSetCurrent(vm->snapshots, current);
>      if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0)
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Snapshots have inconsistent relations for domain %s"),

[...]

> @@ -15927,7 +15923,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>                             _("unable to save metadata for snapshot %s"),
>                             snap->def->name);
>              virDomainSnapshotObjListRemove(vm->snapshots, snap);
> -            vm->current_snapshot = NULL;

virDomainSnapshotSetCurrent(vm->snapshots, NULL);

right?

>          } else {
>              other = virDomainSnapshotFindByName(vm->snapshots,
>                                                  snap->def->parent);

[...]

> 
> @@ -16459,7 +16453,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>       *
>       * XXX Should domain snapshots track live xml rather
>       * than inactive xml?  */
> -    vm->current_snapshot = snap;

virDomainSnapshotSetCurrent(vm->snapshots, snap); ?

>      if (snap->def->dom) {
>          config = virDomainDefCopy(snap->def->dom, caps,
>                                    driver->xmlopt, NULL, true);

[...]

The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
can answer and things will be fine.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John