dafef60 snapshot: Permit redefine of offline external snapshot

Authored and Committed by ericb 5 years ago
    snapshot: Permit redefine of offline external snapshot
    
    Due to historical back-compat, bare '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 with an
    error message that 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 still fails because the
    snapshot XML is not 'disk-only' - making it impossible to recreate
    the snapshot metadata (or to transfer it from one libvirtd host to
    another).  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 using virDomainSnapshotDefIsExternal()
    (caching the results up front, and eliminating other 'if' clauses
    now rendered redundant) when deciding whether the XML being
    requested for redefinition should permit external or force internal
    state capture (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 when the problems were introduced (git blame puts
    some affected hunks as far back as 1.0.0), but it was definitely
    been broken even before 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>
    Reviewed-by: John Ferlan <jferlan@redhat.com>
    
        
file modified
+5 -7