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

On 10/25/2011 02:59 PM, Eric Blake wrote: ... Good thing I haven't pushed yet - I just realized another problem. On Set interfaces, it is the client side that must not provide strings without the flag; there, params is valid on function entry, so we can reject if we see mismatch. Hypervisor callbacks must accept the flag, but need not do anything further with it, because the generic libvirt.c wrapper already checked that the flag is only used when valid. But on Get interfaces, like virDomainGetMemoryParamaters, params is an output-only interface; it can be uninitialized memory on entry, so we must not base our behavior on the client's contents. Rather, the typed parameter check has to either be done by each the callback (where we write each hypervisor driver to not output strings unless they first check flags), or can be factored into libvirt.c (hypervisors can blindly write back all parameters, then libvirt.c does a post-process pass that shrinks the array size to skip past all string entries). From a maintainability perspective, making it so hypervisors don't have to repeat the flag check logic is more appealing. So here's what I'd also like to squash in (don't worry, I'll repost the amended series as a v5, for someone else to do a full review, rather than just this piecemeal stuff): diff --git i/src/libvirt.c w/src/libvirt.c index 379bb20..4d2c7c6 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3579,8 +3579,10 @@ error: return -1; } +/* Helper function called to validate incoming client array on any + * interface that sets typed parameters in the hypervisor. */ static int -virTypedParameterStringCheck(virTypedParameterPtr params, +virTypedParameterValidateSet(virTypedParameterPtr params, int nparams, unsigned int flags) { @@ -3597,6 +3599,26 @@ virTypedParameterStringCheck(virTypedParameterPtr params, return 0; } +/* Helper function called to sanitize outgoing hypervisor array on any + * interface that gets typed parameters for the client. */ +static void +virTypedParameterSanitizeGet(virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + if (params && !(flags & VIR_TYPED_PARAM_STRING_OKAY)) { + int i; + for (i = 0; i < *nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) { + VIR_FREE(params[i].value.s); + memmove(&params[i], &params[i + 1], *nparams - i + 1); + --i; + --*nparams; + } + } + } +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3622,7 +3644,7 @@ virDomainSetMemoryParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3700,9 +3722,6 @@ virDomainGetMemoryParameters(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); @@ -3720,6 +3739,7 @@ virDomainGetMemoryParameters(virDomainPtr domain, ret = conn->driver->domainGetMemoryParameters (domain, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -3754,7 +3774,7 @@ virDomainSetBlkioParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3816,9 +3836,6 @@ 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); @@ -3836,6 +3853,7 @@ virDomainGetBlkioParameters(virDomainPtr domain, ret = conn->driver->domainGetBlkioParameters (domain, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError (VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -6386,9 +6404,6 @@ 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); @@ -6407,6 +6422,7 @@ virDomainGetSchedulerParameters(virDomainPtr domain, ret = conn->driver->domainGetSchedulerParameters (domain, params, nparams); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, 0); return ret; } @@ -6447,9 +6463,6 @@ virDomainGetSchedulerParametersFlags(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); @@ -6469,6 +6482,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } @@ -6504,7 +6518,7 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, 0) < 0) + if (virTypedParameterValidateSet(params, nparams, 0) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -6570,7 +6584,7 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -6714,9 +6728,6 @@ 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); @@ -6733,6 +6744,7 @@ int virDomainBlockStatsFlags (virDomainPtr dom, ret = conn->driver->domainBlockStatsFlags(dom, path, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org