On 10/25/2011 02:59 PM, Eric Blake wrote:
On 10/12/2011 03:26 AM, Hu Tao wrote:
Apologies on the delayed review, but I'm finally getting to this one.
That just goes to show that a lot has
happened in libvirt.c since this patch was proposed.
...
Here's what I'm squashing in:
Good thing I haven't pushed yet - I just realized another problem.
-static int virDomainCheckTypedStringFlag(virTypedParameterPtr params,
- int nparams,
- unsigned int flags)
+static int
+virTypedParameterStringCheck(virTypedParameterPtr params,
+ int nparams,
+ unsigned int flags)
{
@@ -3705,7 +3700,7 @@ virDomainGetMemoryParameters(virDomainPtr domain,
virResetLastError();
- if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0)
+ if (virTypedParameterStringCheck(params, *nparams, flags) < 0)
return -1;
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(¶ms[i], ¶ms[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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list