On 11/02/2011 05:12 AM, Stefan Berger wrote:
On 11/01/2011 07:47 PM, Eric Blake wrote:
Qemu will be the first driver to make use of a typed string in the next round of additions. Separate out the trivial addition.
* src/qemu/qemu_driver.c (qemudSupportsFeature): Advertise feature. (qemuDomainGetBlkioParameters, qemuDomainGetMemoryParameters) (qemuGetSchedulerParametersFlags, qemudDomainBlockStatsFlags): Allow typed strings flag where trivially supported.
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); Here you seem to be mixing flags of different 'enum types', though they don't step on each other. Couldn't you make the VIR_TYPED_PARAM_STRING_OKAY be of the same type as the VIR_DOMAIN_AFFECT_LIVE?
Well, in patch 1/3, libvirt.h.in specifically stated that these enum values must not overlap, but didn't do anything to guarantee that; it's probably worth me adding some verify() codes to ensure sane growth of these enums.
Or do something like this here to prevent the two types of flags from ever stepping on each other:
typedef enum { VIR_TYPED_PARAM_STRING_OKAY = (VIR_DOMAIN_AFFECT_LAST<< 0),
} virTypedParameterFlags;
with VIR_DOMAIN_AFFECT_LAST = (1 << 2).
No, that's not extensible. VIR_DOMAIN_AFFECT_LAST might grow in the future, but VIR_TYPED_PARAM_STRING_OKAY must _not_ change in value, since it is publicly documented. Rather, if we ever add to virDomainModificationImpact, that addition must not overwrite VIR_TYPED_PARAM_STRING_OKAY. Hmm, I'll have to think about how to redo patch 1/3 as a result, to make our intentions clear as to the future growth of either enum value.
Rest looks good. ACK
I'll do some more testing (in particular, actually use the new TYPED_PARAM_STRING by finishing the work on Hu's v4 series to expose cgroups.weight_device through blkio parameters), before posting a v2, since you turned up some good bugs that I should have seen if I had actually used the new code :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org