[libvirt] [PATCH 2/2] qemu: Fix snapshot redefine vs. domain state bug

[Eric Blake]

On 2/25/19 6:01 PM, John Ferlan wrote:
> 
> 
> On 2/23/19 3:00 PM, Eric Blake wrote:
>> The existing qemu snapshot code has a slight bug: if the domain
>> is currently pmsuspended, you can't use the _REDEFINE flag even
>> though the current domain state should have no bearing on being
>> able to recreate metadata state; and conversely, you can use the
>> _REDEFINE flag to create snapshot metadata claiming to be
>> pmsuspended as a bypass to the normal restrictions that you can't
>> create an original qemu snapshot in that state (the restriction
>> against pmsuspend is specific to qemu, rather than part of the
>> driver-agnostic snapshot_conf code).
>>
>> Fix this by factoring out the snapshot validation into a helper
>> routine (which will also be useful in a later patch adding bulk
>> redefine), and by adjusting the state being checked to the one
>> supplied by the snapshot XML for redefinition, vs. the current
>> domain state for original creation.  However, since snapshots
>> have one additional state beyond virDomainState (namely, the
>> "disk-snapshot" state), and given the way virDomainSnapshotState
>> was defined as an extension enum on top of virDomainState, we
>> lose out on gcc's ability to force type-based validation that
>> all switch branches are covered.
> 
> Would it be worth adding some sort of compiler warning that
> VIR_DOMAIN_SNAPSHOT_STATE_LAST must be VIR_DOMAIN_DISK_SNAPSHOT + 1.
> That would cause future additions to virDomainSnapshotState to rework
> the math - verify seems to be used for some places. Perhaps something
> worthwhile for any other one of these overlaid structs.

Well, we already have:

VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_STATE_LAST

Another possibility might be fully flushing out the enum (like we've
done in qemu_blockjob.h with qemuBlockjobState and qemuBlockJobType.

> 
>>
>> In the process, observe that the qemu code already rejects _LIVE
>> with _REDEFINE, but that this rejection occurs after parsing, and
>> with a rather confusing message. Hoist that particular exclusive
>> flag check earlier, so it gets a better error message, and is not
>> inadvertently skipped when bulk redefine is added later (as that
>> addition will occur prior to parsing).
>>
>> Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 117 +++++++++++++++++++++++++----------------
>>  1 file changed, 71 insertions(+), 46 deletions(-)
>>
> 
> It seems there are multiple things going on, some code refactoring and
> then a couple bug fixes. IDC if they're combined - separating for easier
> backporting is always nice...

Now that we've hit rc1, I'll split this patch into smaller pieces, so we
can more easily decide which portions are important for 5.1 and ensure
ease of backports as needed.


> Probably would be good to separate if you think that's wise/feasible.
> Since this is a bug fix it is a change that could be accepted post
> freeze. I'll let you decide how to proceed -
> 
> Reviewed-by: John Ferlan <jferlan at redhat.com>

Look for a v2 to come shortly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org