Re: [libvirt] [PATCHv5 3/3] API: add trivial qemu support for VIR_TYPED_PARAM_STRING
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org