[libvirt] [PATCH 05/16] snapshot: Drop virDomainSnapshotDef.current

[John Ferlan]

On 3/20/19 1:40 AM, Eric Blake wrote:
> The only use for the 'current' member of virDomainSnapshotDef was with
> the PARSE/FORMAT_INTERNAL flag for controlling an internal-use
> <active> element marking whether a particular snapshot definition was
> current, and even then, only by the qemu driver on output, and by qemu
> and test driver on input. But this duplicates vm->snapshot_current,
> and gets in the way of potential simplifications to have qemu store a
> single file for all snapshots rather than one file per snapshot.  Get
> rid of the member by adding a bool* parameter during parse (ignored if
> the PARSE_INTERNAL flag is not set), and by adding a new flag during
> format (if FORMAT_INTERNAL is set, the value printed in <active>
> depends on the new FORMAT_CURRENT).  Then update the qemu driver
> accordingly.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/conf/snapshot_conf.h            |  6 ++--
>  src/conf/snapshot_conf.c            | 18 ++++++----
>  src/conf/virdomainsnapshotobjlist.c |  4 +--
>  src/esx/esx_driver.c                |  2 +-
>  src/qemu/qemu_domain.c              | 18 +++++-----
>  src/qemu/qemu_driver.c              | 51 +++++++++++++++--------------
>  src/test/test_driver.c              |  5 ++-
>  src/vbox/vbox_common.c              |  4 +--
>  src/vz/vz_driver.c                  |  3 +-
>  tests/domainsnapshotxml2xmltest.c   |  5 ++-
>  10 files changed, 66 insertions(+), 50 deletions(-)
> 

[...]

> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
> index ffb1313c89..bf5fdc0647 100644
> --- a/src/conf/snapshot_conf.c
> +++ b/src/conf/snapshot_conf.c
> @@ -184,12 +184,14 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
> 
>  /* flags is bitwise-or of virDomainSnapshotParseFlags.
>   * If flags does not include VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE, then
> - * caps are ignored.
> + * caps are ignored. If flags does not include
> + * VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL, then current is ignored.
>   */
>  static virDomainSnapshotDefPtr
>  virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
>                            virCapsPtr caps,
>                            virDomainXMLOptionPtr xmlopt,
> +                          bool *current,
>                            unsigned int flags)
>  {
>      virDomainSnapshotDefPtr def = NULL;
> @@ -350,7 +352,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
>                             _("Could not find 'active' element"));
>              goto cleanup;
>          }
> -        def->current = active != 0;
> +        *current = active != 0;

Even though we've restricted usage via @flags where PARSE_INTERNAL is
set, should this be prefaced by:

   if (current)

guess I'm concerned with some future cut-copy-paste error...

>      }
> 
>      if (!offline && virSaveCookieParse(ctxt, &def->cookie, saveCookie) < 0)

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

John

The setting of ->current_snapshot in/around calls to
qemuDomainSnapshotWriteMetadata was particularly "interesting" to
follow, but looks fine.

[...]