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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> John [...] -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
