[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]