Re: [libvirt] [PATCH v4 1/2] Add VIR_TYPED_PARAM_STRING

On 10/12/2011 03:26 AM, Hu Tao wrote: Apologies on the delayed review, but I'm finally getting to this one. This patch did not compile for out of the box: libvirt.c: In function 'virDomainSetMemory': libvirt.c:3482:5: error: implicit declaration of function 'virDomainCheckTypedStringFlag' [-Wimplicit-function-declaration] but I think that is how git (mis-)applied the patch based on all the changes that have occurred in the meantime, and the fact that the hunks were contextually ambiguous. Git tried to patch virDomainSetMemoryFlags, which does not use typed parameters, instead of virDomainSetBlkioParameters ;( That just goes to show that a lot has happened in libvirt.c since this patch was proposed. Given that this enum only affects the use of typed parameters, I think it fits slightly better if listed near typed parameters, with a slightly different name. s/confilict/conflict/ virTypedParameters are not domain-specific - it is feasible that we will reuse the type even on the node or network level in the future. I'm dropping _DOMAIN out of the name (and yes, that means I have to rebase a lot of the patch, accordingly - but better to get a good name out of things). mention 0.9.7 as what constitutes new (a year from now, 0.9.7 will feel old :) s/a old/an old/ I reformatted this, for a better 80 column fit. Again, I went with a more generic name: virTypedParameterStringCheck Needs an export in libvirt_private.syms. And unfortunately, it violates the typical vir*Free convention of taking a single parameter, so it's not free-like, but I don't know how to fix that. Here's what I'm squashing in: diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 26a3306..38d1dfb 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -200,6 +200,8 @@ typedef virDomainControlInfo *virDomainControlInfoPtr; * current domain state. VIR_DOMAIN_AFFECT_LIVE requires a running * domain, and VIR_DOMAIN_AFFECT_CONFIG requires a persistent domain * (whether or not it is running). + * + * These enums should not conflict with those of virTypedParameterFlags. */ typedef enum { VIR_DOMAIN_AFFECT_CURRENT = 0, /* Affect current domain state. */ @@ -208,27 +210,6 @@ typedef enum { } virDomainModificationImpact; /** - * virDomainFlags: - * - * Flags that can be used with some libvirt APIs. - * - * These enums should not confilict with those of virDomainModificationImpact. - */ -typedef enum { - VIR_DOMAIN_TYPED_STRING_OKAY = 1 << 2, /* Usage of this flag: - * o new server should not send strings to old client if it - * doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY. - * o new client that wants to be able to send/receive strings - * should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY; - * if it is rejected by a old server that doesn't understand - * VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have - * a second try without flag VIR_DOMAIN_TYPED_STRING_OKAY to - * cope with an old server. - */ - -} virDomainFlags; - -/** * virDomainInfoPtr: * * a virDomainInfo is a structure filled by virDomainGetInfo() and extracting @@ -511,10 +492,31 @@ typedef enum { VIR_TYPED_PARAM_ULLONG = 4, /* unsigned long long case */ VIR_TYPED_PARAM_DOUBLE = 5, /* double case */ VIR_TYPED_PARAM_BOOLEAN = 6, /* boolean(character) case */ - VIR_TYPED_PARAM_STRING = 7, /* string case */ + VIR_TYPED_PARAM_STRING = 7, /* string case; see virTypedParameterFlags */ } virTypedParameterType; /** + * virTypedParameterFlags: + * + * Flags related to libvirt APIs that use virTypedParameter. + * + * These enums should not conflict with those of virDomainModificationImpact. + */ +typedef enum { + /* Usage of this flag: + * - servers before 0.9.7 do not understand typed strings. + * - servers 0.9.7 and newer will not send or receive strings + * without the flag VIR_TYPED_PARAM_STRING_OKAY. + * - clients that want to be able to send/receive strings should + * first try to set the flag VIR_TYPED_PARAM_STRING_OKAY; if + * it is rejected by an old server, then the client can try + * again without the flag, to get only non-string parameters. + */ + VIR_TYPED_PARAM_STRING_OKAY = 1 << 2, + +} virTypedParameterFlags; + +/** * VIR_TYPED_PARAM_FIELD_LENGTH: * * Macro providing the field length of virTypedParameter name diff --git i/src/libvirt.c w/src/libvirt.c index 6e524c3..379bb20 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3479,9 +3479,6 @@ virDomainSetMemory(virDomainPtr domain, unsigned long memory) virResetLastError(); - if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3549,9 +3546,6 @@ virDomainSetMemoryFlags(virDomainPtr domain, unsigned long memory, virResetLastError(); - if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3585,11 +3579,12 @@ error: return -1; } -static int virDomainCheckTypedStringFlag(virTypedParameterPtr params, - int nparams, - unsigned int flags) +static int +virTypedParameterStringCheck(virTypedParameterPtr params, + int nparams, + unsigned int flags) { - if (!(flags & VIR_DOMAIN_TYPED_STRING_OKAY)) { + if (!(flags & VIR_TYPED_PARAM_STRING_OKAY)) { int i; for (i = 0; i < nparams; i++) { if (params[i].type == VIR_TYPED_PARAM_STRING) { @@ -3608,7 +3603,7 @@ static int virDomainCheckTypedStringFlag(virTypedParameterPtr params, * @params: pointer to memory parameter objects * @nparams: number of memory parameter (this value can be the same or * less than the number of parameters supported) - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Change all or a subset of the memory tunables. * This function may require privileged access to the hypervisor. @@ -3627,7 +3622,7 @@ virDomainSetMemoryParameters(virDomainPtr domain, virResetLastError(); - if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) + if (virTypedParameterStringCheck(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3666,7 +3661,7 @@ error: * @params: pointer to memory parameter object * (return value, allocated by the caller) * @nparams: pointer to number of memory parameters - * @flags: one of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get all memory parameters, the @params array will be filled with the values * equal to the number of parameters suggested by @nparams @@ -3705,7 +3700,7 @@ virDomainGetMemoryParameters(virDomainPtr domain, virResetLastError(); - if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) + if (virTypedParameterStringCheck(params, *nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3740,7 +3735,7 @@ error: * @params: pointer to blkio parameter objects * @nparams: number of blkio parameters (this value can be the same or * less than the number of parameters supported) - * @flags: an OR'ed set of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Change all or a subset of the blkio tunables. * This function may require privileged access to the hypervisor. @@ -3759,6 +3754,9 @@ virDomainSetBlkioParameters(virDomainPtr domain, virResetLastError(); + if (virTypedParameterStringCheck(params, nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3795,7 +3793,7 @@ error: * @params: pointer to blkio parameter object * (return value, allocated by the caller) * @nparams: pointer to number of blkio parameters - * @flags: an OR'ed set of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get all blkio parameters, the @params array will be filled with the values * equal to the number of parameters suggested by @nparams. @@ -3818,6 +3816,9 @@ virDomainGetBlkioParameters(virDomainPtr domain, virResetLastError(); + if (virTypedParameterStringCheck(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6336,9 +6337,6 @@ virDomainGetSchedulerType(virDomainPtr domain, int *nparams) virResetLastError(); - if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6388,6 +6386,9 @@ virDomainGetSchedulerParameters(virDomainPtr domain, virResetLastError(); + if (virTypedParameterStringCheck(params, *nparams, 0) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6424,7 +6425,7 @@ error: * @nparams: pointer to number of scheduler parameter * (this value should be same than the returned value * nparams of virDomainGetSchedulerType) - * @flags: one of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Get the scheduler parameters, the @params array will be filled with the * values. @@ -6446,7 +6447,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); - if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) + if (virTypedParameterStringCheck(params, *nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -6503,6 +6504,9 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virResetLastError(); + if (virTypedParameterStringCheck(params, nparams, 0) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6543,7 +6547,7 @@ error: * @nparams: number of scheduler parameter objects * (this value can be the same or less than the returned value * nparams of virDomainGetSchedulerType) - * @flags: bitwise-OR of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags * * Change a subset or all scheduler parameters. The value of @flags * should be either VIR_DOMAIN_AFFECT_CURRENT, or a bitwise-or of @@ -6566,6 +6570,9 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); + if (virTypedParameterStringCheck(params, nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6636,9 +6643,6 @@ virDomainBlockStats (virDomainPtr dom, const char *path, virResetLastError(); - if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN (dom)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6672,7 +6676,7 @@ error: * @params: pointer to block stats parameter object * (return value) * @nparams: pointer to number of block stats - * @flags: unused, always passes 0 + * @flags: bitwise-OR of virTypedParameterFlags * * This function is to get block stats parameters for block * devices attached to the domain. @@ -6710,6 +6714,9 @@ int virDomainBlockStatsFlags (virDomainPtr dom, virResetLastError(); + if (virTypedParameterStringCheck(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); diff --git i/src/util/util.c w/src/util/util.c index 895e6b1..506d355 100644 --- i/src/util/util.c +++ w/src/util/util.c @@ -2571,7 +2571,8 @@ or other application using the libvirt API.\n\ return 0; } -void virTypedParameterFree(virTypedParameterPtr params, int nparams) +void +virTypedParameterFree(virTypedParameterPtr params, int nparams) { int i; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org