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

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.

> > 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(a)redhat.com&gt; > --- > 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...

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(a)redhat.com&gt;