Due to historical back-compat, 'virsh snapshot-create-as' favors
internal snapshots (but can't be used on domains with raw storage),
while 'virsh snapshot-create-as --disk-only) favors external
snapshots.  What's more, snapshots created with --disk-only while
the domain was running are marked as snapshot state 'disk-snapshot',
while snapshots created while the domain was offline are marked as
snapshot state 'shutdown' (a 'disk-snapshot' image might not be
quiescent, while a 'shutdown' snapshot always is).

But this leads to some interesting problems: if we create a
--disk-only snapshot of an offline guest, and then immediately try
to 'virsh snapshot-create --redefine' using the resulting XML to
overwrite the existing snapashot in place, things silently succeed,
but 'virsh snapshot-create --redefine --disk-only' fails because
the snapshot state is not 'disk-only'.  Worse, if we delete the
snapshot metadata first and then try to recreate things, omitting
--disk-only fails because the verification code wants to force the
default of an internal snapshot (which doesn't work with raw disks),
and using --disk-only fails because the verification code doesn't
see the 'disk-only' state in the snapshot xml - making it impossible
to recreate the snapshot metadata.  Ideally, the presence or absence
of the --disk-only flag, and the presence or absence of an existing
snapshot being overwritten, shouldn't matter; if the XML is valid
for one situation, it should always be valid to redefine the
metadata for that snapshot.

Fix things by uniformly declaring that a redefined snapshot in the
'shutdown' state can permit a disk-only external snapshot (we got
it right in only one out of three places in the function).

See also https://bugzilla.redhat.com/1680304; this fixes the
domain-agnostic problems mentioned there, but another patch is needed
to fix further oddities with the qemu driver.  I did not check for
sure, but this has probably been broken even prior to when commit
670e86bf (1.1.4) factored redefine prep out of qemu code into the
common snapshot_conf code.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/conf/snapshot_conf.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 489c25f511..3372c4df86 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1225,6 +1225,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
     int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
     bool align_match = true;
     virDomainSnapshotObjPtr other;
+    bool offline = def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+        virDomainSnapshotDefIsExternal(def);

     /* Prevent circular chains */
     if (def->parent) {
@@ -1259,14 +1261,12 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
     }

     /* Check that any replacement is compatible */
-    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
-        def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
+    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !offline) {
         virReportError(VIR_ERR_INVALID_ARG,
                        _("disk-only flag for snapshot %s requires "
                          "disk-snapshot state"),
                        def->name);
         goto cleanup;
-
     }

     if (def->dom &&
@@ -1315,8 +1315,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
         }

         if (def->dom) {
-            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
-                virDomainSnapshotDefIsExternal(def)) {
+            if (offline) {
                 align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
                 align_match = false;
             }
@@ -1346,7 +1345,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
         *snap = other;
     } else {
         if (def->dom) {
-            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+            if (offline ||
                 def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
                 align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
                 align_match = false;
-- 
2.20.1

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

On 2/23/19 3:00 PM, Eric Blake wrote:
> Due to historical back-compat, 'virsh snapshot-create-as' favors
> internal snapshots (but can't be used on domains with raw storage),
> while 'virsh snapshot-create-as --disk-only) favors external

s/)/'

> snapshots.  What's more, snapshots created with --disk-only while
> the domain was running are marked as snapshot state 'disk-snapshot',
> while snapshots created while the domain was offline are marked as
> snapshot state 'shutdown' (a 'disk-snapshot' image might not be
> quiescent, while a 'shutdown' snapshot always is).
> 
> But this leads to some interesting problems: if we create a
> --disk-only snapshot of an offline guest, and then immediately try
> to 'virsh snapshot-create --redefine' using the resulting XML to
> overwrite the existing snapashot in place, things silently succeed,
> but 'virsh snapshot-create --redefine --disk-only' fails because
> the snapshot state is not 'disk-only'.  Worse, if we delete the
> snapshot metadata first and then try to recreate things, omitting
> --disk-only fails because the verification code wants to force the
> default of an internal snapshot (which doesn't work with raw disks),
> and using --disk-only fails because the verification code doesn't
> see the 'disk-only' state in the snapshot xml - making it impossible
> to recreate the snapshot metadata.  Ideally, the presence or absence
> of the --disk-only flag, and the presence or absence of an existing
> snapshot being overwritten, shouldn't matter; if the XML is valid
> for one situation, it should always be valid to redefine the
> metadata for that snapshot.
> 
> Fix things by uniformly declaring that a redefined snapshot in the
> 'shutdown' state can permit a disk-only external snapshot (we got
> it right in only one out of three places in the function).
> 
> See also https://bugzilla.redhat.com/1680304; this fixes the
> domain-agnostic problems mentioned there, but another patch is needed
> to fix further oddities with the qemu driver.  I did not check for
> sure, but this has probably been broken even prior to when commit
> 670e86bf (1.1.4) factored redefine prep out of qemu code into the
> common snapshot_conf code.
> 

Perhaps 709b0f37c (1.0.2)? or older e260e401a5 (1.0.0)

The 1.0.2 change seems to be the source of hunk 2 and 3 below, while
1.0.0 is the source of hunk 1.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/conf/snapshot_conf.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 

Your explanation seems reasonable and although I don't think I could
begin to (re) explain all the various states, conditions,
[in]consistencies, etc... Still, consider it

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

John

[...]

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