On 05/21/2010 12:48 PM, Eric Blake wrote:
On 05/21/2010 07:22 AM, Chris Lalancette wrote:
>>> - virCheckFlags(VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC,
>>> - -1);
>>> -
>>
>> Why are you removing the valid flag check altogether? Is it a matter of
>> adding more valid flags, or is this just a forwarding routine to other
>> methods that also do a valid flag check?
>
> Yeah, I guess I should have been a bit more explicit here. There's no
hard-and-fast
> rules about virCheckFlags(), but the general usage of it seems to be directly at
> the entry points to the driver (i.e. in qemuDomainSuspend(), etc).
doNativeMigrate()
> is not an entry point, it is a helper function. In addition, there is a long list
> of flags that migration supports, so the above list is far from complete. Would it
> be better if I added the check to qemuDomainMigratePerform() (with the complete
> list of flags), which is actually the entry point for this function?
Sounds good to me - if all entry points filter on all accepted flags,
then helper functions can assume that flags are already valid. As long
as the filtering gets done somewhere, we've left the door open for
adding future flags while still correctly identifying situations where
we are talking to older implementations that can't honor new flags.
It's only when there is no flag filtering at all that we've locked
ourselves out of easy-to-validate future extensions.
Unfortunately doing this caused a bit of churn in the qemu driver. qemudDomainMigrate
takes an unsigned long as flags instead of unsigned int, which required me to create
two new macros: virCheckFlagsUI and virCheckFlagsUL. The good news is that with this
patch in place we are doing more checking of the flags during migration, which is
probably a good thing. A new patch is coming up.
--
Chris Lalancette