6:47 p.m.
This builds on v4:
https://www.redhat.com/archives/libvir-list/2011-October/msg00446.html
It only implements the front end of supporting typed strings across
an RPC call; I'm still working on refactoring the back half of the
v4 series to actually wire up blkio device weights to be the first
use that actually sends a string, but wanted to get the review
started on these now.
Changes from v4:
Added a driver feature bit. With that, it is no longer necessary
to set a flag when setting parameters (libvirt.c rejects attempts
for a new client to send strings to an old server lacking the
feature), and getting strings no longer needs user interactions
(the user does not have to worry about the flag bit; either the
client is too old so the flag is not set, or the client is
new enough and libvirt.c probes to see if the driver supports
things to auto-set the flag; also, libvirt.c takes care of
filtering out strings on the server side before returning so
that the drivers do not have to pay much attention to the flag).
Eric Blake (3):
API: add VIR_TYPED_PARAM_STRING
API: remote support for VIR_TYPED_PARAM_STRING
API: add trivial qemu support for VIR_TYPED_PARAM_STRING
daemon/remote.c | 33 ++++++++++--
include/libvirt/libvirt.h.in | 28 ++++++++++-
src/libvirt.c | 119 +++++++++++++++++++++++++++++++++++++-----
src/libvirt_internal.h | 9 +++-
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 24 +++++++--
src/remote/remote_driver.c | 28 +++++++++-
src/remote/remote_protocol.x | 2 +
src/remote_protocol-structs | 2 +
src/rpc/gendispatch.pl | 3 +-
src/util/util.c | 14 +++++
src/util/util.h | 2 +
12 files changed, 237 insertions(+), 28 deletions(-)
--
1.7.4.4
6:47 p.m.
New subject: [libvirt] [PATCHv5 1/3] API: add VIR_TYPED_PARAM_STRING
This allows strings to be transported between client and server
in the context of name-type-value virTypedParameter functions.
For compatibility,
o new clients will not send strings to old servers, based on
a feature check
o new servers will not send strings to old clients without the
flag VIR_TYPED_PARAM_STRING_OKAY; this is enforced by
libvirt.c, so that drivers need not worry about it
o the flag VIR_TYPED_PARAM_STRING_OKAY is set automatically,
based on a feature check (so far, no driver implements it),
so clients do not have to worry about it
Future patches can then enable the feature on a per-driver basis.
This patch also ensures that drivers can blindly strdup() field
names (previously, a malicious client could stuff 80 non-NUL bytes
into field and cause a read overrun).
* src/libvirt_internal.h (VIR_DRV_FEATURE_TYPED_PARAM_STRING): New
driver feature.
* src/libvirt.c (virTypedParameterValidateSet)
(virTypedParameterSanitizeGet): New helper functions.
(virDomainSetMemoryParameters, virDomainSetBlkioParameters)
(virDomainSetSchedulerParameters)
(virDomainSetSchedulerParametersFlags)
(virDomainGetMemoryParameters, virDomainGetBlkioParameters)
(virDomainGetSchedulerParameters)
(virDomainGetSchedulerParametersFlags, virDomainBlockStatsFlags):
Use them.
* src/util/util.h (virTypedParameterArrayClear): New helper
function.
* src/util/util.c (virTypedParameterArrayClear): Implement it.
* src/libvirt_private.syms (util.h): Export it.
Based on an initial patch by Hu Tao, with feedback from
Daniel P. Berrange.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt.h.in | 28 ++++++++++-
src/libvirt.c | 119 +++++++++++++++++++++++++++++++++++++-----
src/libvirt_internal.h | 9 +++-
src/libvirt_private.syms | 1 +
src/util/util.c | 14 +++++
src/util/util.h | 2 +
6 files changed, 156 insertions(+), 17 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 0840d46..c737e72 100644
--- a/include/libvirt/libvirt.h.in
+++ b/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. */
@@ -489,10 +491,33 @@ typedef enum {
VIR_TYPED_PARAM_LLONG = 3, /* long long case */
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_BOOLEAN = 6, /* boolean(character) case */
+ VIR_TYPED_PARAM_STRING = 7, /* string case */
} virTypedParameterType;
/**
+ * virTypedParameterFlags:
+ *
+ * Flags related to libvirt APIs that use virTypedParameter.
+ *
+ * These enums should not conflict with those of virDomainModificationImpact.
+ */
+typedef enum {
+ /* Older servers lacked the ability to handle string typed
+ * parameters. Attempts to set a string parameter with an older
+ * server will fail at the client, but attempts to retrieve
+ * parameters must not return strings from a new server to an
+ * older client, so this flag exists to identify newer clients to
+ * newer servers. This flag is automatically set when needed, so
+ * the user does not have to worry about it; however, manually
+ * setting the flag can be used to reject servers that cannot
+ * return typed strings, even if no strings would be returned.
+ */
+ VIR_TYPED_PARAM_STRING_OKAY = 1 << 2,
+
+} virTypedParameterFlags;
+
+/**
* VIR_TYPED_PARAM_FIELD_LENGTH:
*
* Macro providing the field length of virTypedParameter name
@@ -520,6 +545,7 @@ struct _virTypedParameter {
unsigned long long int ul; /* type is ULLONG */
double d; /* type is DOUBLE */
char b; /* type is BOOLEAN */
+ char *s; /* type is STRING, may not be NULL */
} value; /* parameter value */
};
diff --git a/src/libvirt.c b/src/libvirt.c
index b0d1e01..5041d88 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -3583,6 +3583,71 @@ error:
return -1;
}
+/* Helper function called to validate incoming client array on any
+ * interface that sets typed parameters in the hypervisor. */
+static int
+virTypedParameterValidateSet(virDomainPtr domain,
+ virTypedParameterPtr params,
+ int nparams)
+{
+ bool string_okay;
+ int i;
+
+ string_okay = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
+ domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING);
+ for (i = 0; i < nparams; i++) {
+ if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) ==
+ VIR_TYPED_PARAM_FIELD_LENGTH) {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("string parameter name '%.*s' too
long"),
+ VIR_TYPED_PARAM_FIELD_LENGTH,
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ if (params[i].type == VIR_TYPED_PARAM_STRING) {
+ if (string_okay) {
+ if (!params[i].value.s) {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("NULL string parameter '%s'"),
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ } else {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("string parameter '%s'
unsupported"),
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ }
+ }
+ 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;
+ memset(¶ms[*nparams], 0, sizeof(*params));
+ }
+ }
+ }
+}
+
/**
* virDomainSetMemoryParameters:
* @domain: pointer to domain object
@@ -3621,6 +3686,9 @@ virDomainSetMemoryParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams) < 0)
+ return -1;
+
conn = domain->conn;
if (conn->driver->domainSetMemoryParameters) {
@@ -3644,7 +3712,7 @@ error:
* @params: pointer to memory parameter object
* (return value, allocated by the caller)
* @nparams: pointer to number of memory parameters; input and output
- * @flags: one of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all memory parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -3695,6 +3763,9 @@ virDomainGetMemoryParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetMemoryParameters) {
@@ -3702,6 +3773,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__);
@@ -3717,7 +3789,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
*
* Change all or a subset of the blkio tunables.
* This function may require privileged access to the hypervisor.
@@ -3749,6 +3821,9 @@ virDomainSetBlkioParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams) < 0)
+ return -1;
+
conn = domain->conn;
if (conn->driver->domainSetBlkioParameters) {
@@ -3772,7 +3847,7 @@ error:
* @params: pointer to blkio parameter object
* (return value, allocated by the caller)
* @nparams: pointer to number of blkio parameters; input and output
- * @flags: an OR'ed set of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all blkio parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -3814,6 +3889,9 @@ virDomainGetBlkioParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetBlkioParameters) {
@@ -3821,6 +3899,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__);
@@ -6384,7 +6463,6 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
-
conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) {
@@ -6392,6 +6470,7 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
ret = conn->driver->domainGetSchedulerParameters (domain, params,
nparams);
if (ret < 0)
goto error;
+ virTypedParameterSanitizeGet(params, nparams, 0);
return ret;
}
@@ -6410,7 +6489,7 @@ error:
* @nparams: pointer to number of scheduler parameter
* (this value should be same than the returned value
* nparams of virDomainGetSchedulerType()); input and output
- * @flags: one of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all scheduler parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -6456,6 +6535,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetSchedulerParametersFlags) {
@@ -6464,6 +6546,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
nparams, flags);
if (ret < 0)
goto error;
+ virTypedParameterSanitizeGet(params, nparams, flags);
return ret;
}
@@ -6505,15 +6588,17 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
return -1;
}
+ if (domain->conn->flags & VIR_CONNECT_RO) {
+ virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+ goto error;
+ }
if (params == NULL || nparams < 0) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams) < 0)
+ return -1;
- if (domain->conn->flags & VIR_CONNECT_RO) {
- virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
- goto error;
- }
conn = domain->conn;
if (conn->driver->domainSetSchedulerParameters) {
@@ -6568,15 +6653,17 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain,
return -1;
}
+ if (domain->conn->flags & VIR_CONNECT_RO) {
+ virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+ goto error;
+ }
if (params == NULL || nparams < 0) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams) < 0)
+ return -1;
- if (domain->conn->flags & VIR_CONNECT_RO) {
- virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
- goto error;
- }
conn = domain->conn;
if (conn->driver->domainSetSchedulerParametersFlags) {
@@ -6665,7 +6752,7 @@ error:
* @params: pointer to block stats parameter object
* (return value)
* @nparams: pointer to number of block stats; input and output
- * @flags: unused, always pass 0
+ * @flags: bitwise-OR of virTypedParameterFlags
*
* This function is to get block stats parameters for block
* devices attached to the domain.
@@ -6715,6 +6802,9 @@ int virDomainBlockStatsFlags(virDomainPtr dom,
virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = dom->conn;
if (conn->driver->domainBlockStatsFlags) {
@@ -6722,6 +6812,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__);
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 0117c5b..2550d76 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -1,7 +1,7 @@
/*
* libvirt.h: publically exported APIs, not for public use
*
- * Copyright (C) 2006-2008 Red Hat, Inc.
+ * Copyright (C) 2006-2008, 2011 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -83,7 +83,12 @@ enum {
/*
* Support for file descriptor passing
*/
- VIR_DRV_FEATURE_FD_PASSING = 8
+ VIR_DRV_FEATURE_FD_PASSING = 8,
+
+ /*
+ * Support for VIR_TYPED_PARAM_STRING
+ */
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING = 9,
};
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6a1562e..2185294 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1162,6 +1162,7 @@ virStrncpy;
virTimeMs;
virTimestamp;
virTrimSpaces;
+virTypedParameterArrayClear;
virVasprintf;
diff --git a/src/util/util.c b/src/util/util.c
index bd52b21..b25f8db 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2572,3 +2572,17 @@ or other application using the libvirt API.\n\
return 0;
}
+
+void
+virTypedParameterArrayClear(virTypedParameterPtr params, int nparams)
+{
+ int i;
+
+ if (!params)
+ return;
+
+ for (i = 0; i < nparams; i++) {
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[i].value.s);
+ }
+}
diff --git a/src/util/util.h b/src/util/util.h
index e869f1b..e5594cb 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -263,4 +263,6 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1);
int virEmitXMLWarning(int fd,
const char *name,
const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+
+void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
#endif /* __VIR_UTIL_H__ */
--
1.7.4.4
5:49 a.m.
New subject: [libvirt] [PATCHv5 1/3] API: add VIR_TYPED_PARAM_STRING
On 11/01/2011 07:47 PM, Eric Blake wrote:
This allows strings to be transported between client and server
in the context of name-type-value virTypedParameter functions.
For compatibility,
o new clients will not send strings to old servers, based on
a feature check
o new servers will not send strings to old clients without the
flag VIR_TYPED_PARAM_STRING_OKAY; this is enforced by
libvirt.c, so that drivers need not worry about it
o the flag VIR_TYPED_PARAM_STRING_OKAY is set automatically,
based on a feature check (so far, no driver implements it),
so clients do not have to worry about it
Future patches can then enable the feature on a per-driver basis.
This patch also ensures that drivers can blindly strdup() field
names (previously, a malicious client could stuff 80 non-NUL bytes
into field and cause a read overrun).
* src/libvirt_internal.h (VIR_DRV_FEATURE_TYPED_PARAM_STRING): New
driver feature.
* src/libvirt.c (virTypedParameterValidateSet)
(virTypedParameterSanitizeGet): New helper functions.
(virDomainSetMemoryParameters, virDomainSetBlkioParameters)
(virDomainSetSchedulerParameters)
(virDomainSetSchedulerParametersFlags)
(virDomainGetMemoryParameters, virDomainGetBlkioParameters)
(virDomainGetSchedulerParameters)
(virDomainGetSchedulerParametersFlags, virDomainBlockStatsFlags):
Use them.
* src/util/util.h (virTypedParameterArrayClear): New helper
function.
* src/util/util.c (virTypedParameterArrayClear): Implement it.
* src/libvirt_private.syms (util.h): Export it.
Based on an initial patch by Hu Tao, with feedback from
Daniel P. Berrange.
Signed-off-by: Eric Blake<eblake(a)redhat.com>
---
include/libvirt/libvirt.h.in | 28 ++++++++++-
src/libvirt.c | 119 +++++++++++++++++++++++++++++++++++++-----
src/libvirt_internal.h | 9 +++-
src/libvirt_private.syms | 1 +
src/util/util.c | 14 +++++
src/util/util.h | 2 +
6 files changed, 156 insertions(+), 17 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 0840d46..c737e72 100644
--- a/include/libvirt/libvirt.h.in
+++ b/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. */
@@ -489,10 +491,33 @@ typedef enum {
VIR_TYPED_PARAM_LLONG = 3, /* long long case */
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_BOOLEAN = 6, /* boolean(character) case */
+ VIR_TYPED_PARAM_STRING = 7, /* string case */
} virTypedParameterType;
/**
+ * virTypedParameterFlags:
+ *
+ * Flags related to libvirt APIs that use virTypedParameter.
+ *
+ * These enums should not conflict with those of virDomainModificationImpact.
+ */
+typedef enum {
+ /* Older servers lacked the ability to handle string typed
+ * parameters. Attempts to set a string parameter with an older
+ * server will fail at the client, but attempts to retrieve
+ * parameters must not return strings from a new server to an
+ * older client, so this flag exists to identify newer clients to
+ * newer servers. This flag is automatically set when needed, so
+ * the user does not have to worry about it; however, manually
+ * setting the flag can be used to reject servers that cannot
+ * return typed strings, even if no strings would be returned.
+ */
+ VIR_TYPED_PARAM_STRING_OKAY = 1<< 2,
+
+} virTypedParameterFlags;
+
+/**
* VIR_TYPED_PARAM_FIELD_LENGTH:
*
* Macro providing the field length of virTypedParameter name
@@ -520,6 +545,7 @@ struct _virTypedParameter {
unsigned long long int ul; /* type is ULLONG */
double d; /* type is DOUBLE */
char b; /* type is BOOLEAN */
+ char *s; /* type is STRING, may not be NULL */
} value; /* parameter value */
};
diff --git a/src/libvirt.c b/src/libvirt.c
index b0d1e01..5041d88 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -3583,6 +3583,71 @@ error:
return -1;
}
+/* Helper function called to validate incoming client array on any
+ * interface that sets typed parameters in the hypervisor. */
+static int
+virTypedParameterValidateSet(virDomainPtr domain,
+ virTypedParameterPtr params,
+ int nparams)
+{
+ bool string_okay;
+ int i;
+
+ string_okay = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver,
+ domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING);
+ for (i = 0; i< nparams; i++) {
+ if (strnlen(params[i].field, VIR_TYPED_PARAM_FIELD_LENGTH) ==
+ VIR_TYPED_PARAM_FIELD_LENGTH) {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("string parameter name '%.*s' too
long"),
+ VIR_TYPED_PARAM_FIELD_LENGTH,
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ if (params[i].type == VIR_TYPED_PARAM_STRING) {
+ if (string_okay) {
+ if (!params[i].value.s) {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("NULL string parameter
'%s'"),
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ } else {
+ virLibDomainError(VIR_ERR_INVALID_ARG,
+ _("string parameter '%s'
unsupported"),
+ params[i].field);
+ virDispatchError(NULL);
+ return -1;
+ }
+ }
+ }
+ 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);
*nparams = 5
i = 0
-> 5-0+1 = 6 -> you would move too many elements (but not enough bytes)
It should be '(*nparams - i - 1) * sizeof(*params)' !
+ --i;
+ --*nparams;
+ memset(¶ms[*nparams], 0, sizeof(*params));
+ }
+ }
+ }
+}
+
/**
* virDomainSetMemoryParameters:
* @domain: pointer to domain object
@@ -3621,6 +3686,9 @@ virDomainSetMemoryParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams)< 0)
+ return -1;
+
conn = domain->conn;
if (conn->driver->domainSetMemoryParameters) {
@@ -3644,7 +3712,7 @@ error:
* @params: pointer to memory parameter object
* (return value, allocated by the caller)
* @nparams: pointer to number of memory parameters; input and output
- * @flags: one of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all memory parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -3695,6 +3763,9 @@ virDomainGetMemoryParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetMemoryParameters) {
@@ -3702,6 +3773,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__);
@@ -3717,7 +3789,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
*
* Change all or a subset of the blkio tunables.
* This function may require privileged access to the hypervisor.
@@ -3749,6 +3821,9 @@ virDomainSetBlkioParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams)< 0)
+ return -1;
+
conn = domain->conn;
if (conn->driver->domainSetBlkioParameters) {
@@ -3772,7 +3847,7 @@ error:
* @params: pointer to blkio parameter object
* (return value, allocated by the caller)
* @nparams: pointer to number of blkio parameters; input and output
- * @flags: an OR'ed set of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all blkio parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -3814,6 +3889,9 @@ virDomainGetBlkioParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetBlkioParameters) {
@@ -3821,6 +3899,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__);
@@ -6384,7 +6463,6 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
-
conn = domain->conn;
if (conn->driver->domainGetSchedulerParameters) {
@@ -6392,6 +6470,7 @@ virDomainGetSchedulerParameters(virDomainPtr domain,
ret = conn->driver->domainGetSchedulerParameters (domain, params,
nparams);
if (ret< 0)
goto error;
+ virTypedParameterSanitizeGet(params, nparams, 0);
return ret;
}
@@ -6410,7 +6489,7 @@ error:
* @nparams: pointer to number of scheduler parameter
* (this value should be same than the returned value
* nparams of virDomainGetSchedulerType()); input and output
- * @flags: one of virDomainModificationImpact
+ * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags
*
* Get all scheduler parameters. On input, @nparams gives the size of the
* @params array; on output, @nparams gives how many slots were filled
@@ -6456,6 +6535,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = domain->conn;
if (conn->driver->domainGetSchedulerParametersFlags) {
@@ -6464,6 +6546,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain,
nparams, flags);
if (ret< 0)
goto error;
+ virTypedParameterSanitizeGet(params, nparams, flags);
return ret;
}
@@ -6505,15 +6588,17 @@ virDomainSetSchedulerParameters(virDomainPtr domain,
return -1;
}
+ if (domain->conn->flags& VIR_CONNECT_RO) {
+ virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+ goto error;
+ }
if (params == NULL || nparams< 0) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams)< 0)
+ return -1;
- if (domain->conn->flags& VIR_CONNECT_RO) {
- virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
- goto error;
- }
conn = domain->conn;
if (conn->driver->domainSetSchedulerParameters) {
@@ -6568,15 +6653,17 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain,
return -1;
}
+ if (domain->conn->flags& VIR_CONNECT_RO) {
+ virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
+ goto error;
+ }
if (params == NULL || nparams< 0) {
virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (virTypedParameterValidateSet(domain, params, nparams)< 0)
+ return -1;
- if (domain->conn->flags& VIR_CONNECT_RO) {
- virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__);
- goto error;
- }
conn = domain->conn;
if (conn->driver->domainSetSchedulerParametersFlags) {
@@ -6665,7 +6752,7 @@ error:
* @params: pointer to block stats parameter object
* (return value)
* @nparams: pointer to number of block stats; input and output
- * @flags: unused, always pass 0
+ * @flags: bitwise-OR of virTypedParameterFlags
*
* This function is to get block stats parameters for block
* devices attached to the domain.
@@ -6715,6 +6802,9 @@ int virDomainBlockStatsFlags(virDomainPtr dom,
virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
goto error;
}
+ if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
conn = dom->conn;
if (conn->driver->domainBlockStatsFlags) {
@@ -6722,6 +6812,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__);
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 0117c5b..2550d76 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -1,7 +1,7 @@
/*
* libvirt.h: publically exported APIs, not for public use
*
- * Copyright (C) 2006-2008 Red Hat, Inc.
+ * Copyright (C) 2006-2008, 2011 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -83,7 +83,12 @@ enum {
/*
* Support for file descriptor passing
*/
- VIR_DRV_FEATURE_FD_PASSING = 8
+ VIR_DRV_FEATURE_FD_PASSING = 8,
+
+ /*
+ * Support for VIR_TYPED_PARAM_STRING
+ */
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING = 9,
};
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 6a1562e..2185294 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1162,6 +1162,7 @@ virStrncpy;
virTimeMs;
virTimestamp;
virTrimSpaces;
+virTypedParameterArrayClear;
virVasprintf;
diff --git a/src/util/util.c b/src/util/util.c
index bd52b21..b25f8db 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2572,3 +2572,17 @@ or other application using the libvirt API.\n\
return 0;
}
+
+void
+virTypedParameterArrayClear(virTypedParameterPtr params, int nparams)
+{
+ int i;
+
+ if (!params)
+ return;
+
+ for (i = 0; i< nparams; i++) {
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[i].value.s);
+ }
+}
diff --git a/src/util/util.h b/src/util/util.h
index e869f1b..e5594cb 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -263,4 +263,6 @@ bool virIsDevMapperDevice(const char *dev_name)
ATTRIBUTE_NONNULL(1);
int virEmitXMLWarning(int fd,
const char *name,
const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+
+void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams);
#endif /* __VIR_UTIL_H__ */
Rest looks good. With nit fixed ACK.
7:18 p.m.
New subject: [libvirt] [PATCHv5 1/3] API: add VIR_TYPED_PARAM_STRING
On 11/01/2011 05:47 PM, Eric Blake wrote:
This allows strings to be transported between client and server
in the context of name-type-value virTypedParameter functions.
For compatibility,
o new clients will not send strings to old servers, based on
a feature check
o new servers will not send strings to old clients without the
flag VIR_TYPED_PARAM_STRING_OKAY; this is enforced by
libvirt.c, so that drivers need not worry about it
o the flag VIR_TYPED_PARAM_STRING_OKAY is set automatically,
based on a feature check (so far, no driver implements it),
so clients do not have to worry about it
Future patches can then enable the feature on a per-driver basis.
Now that I've finally got blkiotune passing strings coded up, I found
out my back-compat rules above didn't quite work; I'll have to fix it in
v6. Basically, we have the following call chains when using qemu:
new virsh
virDomainGetBlkioParameters on client
probe for feature in rpc driver
makes rpc call, remote server supports it
driver callback with STRING_OKAY flag to rpc driver
makes rpc call
virDomainGetBlkioParameters on server
probe for feature in qemu driver
flag already set
makes driver call with STRING_OKAY flag
filter has nothing to remove
rpc call returns string
filter has nothing to remove
success, virsh sees string param
old virsh
virDomainGetBlkioParameters on client
driver callback without STRING_OKAY flag to rpc driver
makes rpc call
virDomainGetBlkioParameters on server
probe for feature in qemu driver
flag was clear, now gets set
makes driver call with STRING_OKAY flag
filter has nothing to remove
rpc call returns string
not expecting string, rpc rejects reply
virsh sees error
So, while libvirt.c can auto-add the flag, the filtering should be done
in the rpc return code, rather than in libvirt.c. That is, libvirt.c
doesn't know whether it is being called by a local client (which means
the client was compiled against new enough headers and supports the
returned string) or a remote client (in which case
virDomainGetBlkioParameters was called by the rpc code, so the rpc code
knows whether the remote client passed the flag, but libvirt.c always
sees the flag set). The corrected call chains will look like:
new virsh
virDomainGetBlkioParameters on client
probe for feature in rpc driver
makes rpc call, remote server supports it
driver callback with STRING_OKAY flag to rpc driver
makes rpc call
virDomainGetBlkioParameters on server
probe for feature in qemu driver
flag already set
makes driver call with STRING_OKAY flag
rpc call saw flag set on entry, so it returns string
rpc client unpacks string
success, virsh sees string param
old virsh
virDomainGetBlkioParameters on client
driver callback without STRING_OKAY flag to rpc driver
makes rpc call
virDomainGetBlkioParameters on server
probe for feature in qemu driver
flag was clear, now gets set
makes driver call with STRING_OKAY flag
rpc call saw flag clear on entry, filters string
rpc client does not have to deal with stringrpc rejects reply
success, virsh does not see string param
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:47 p.m.
New subject: [libvirt] [PATCHv5 2/3] API: remote support for VIR_TYPED_PARAM_STRING
Send and receive string typed parameters across RPC.
* src/remote/remote_protocol.x (remote_typed_param_value): Add
another union value.
* daemon/remote.c (remoteSerializeTypedParameters)
(remoteDeserializeTypedParameters): Handle strings on rpc.
* src/remote/remote_driver.c (remoteFreeTypedParameters)
(remoteSerializeTypedParameters)
(remoteDeserializeTypedParameters): Likewise.
* src/rpc/gendispatch.pl: Properly clean up typed arrays.
* src/remote_protocol-structs: Update.
Based on an initial patch by Hu Tao, with feedback from
Daniel P. Berrange.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
daemon/remote.c | 33 +++++++++++++++++++++++++++++----
src/remote/remote_driver.c | 28 ++++++++++++++++++++++++++--
src/remote/remote_protocol.x | 2 ++
src/remote_protocol-structs | 2 ++
src/rpc/gendispatch.pl | 3 ++-
5 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index bd0c3e3..e3a9775 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -701,7 +701,7 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
for (i = 0; i < nparams; ++i) {
/* remoteDispatchClientRequest will free this: */
- val[i].field = strdup (params[i].field);
+ val[i].field = strdup(params[i].field);
if (val[i].field == NULL) {
virReportOOMError();
goto cleanup;
@@ -726,6 +726,13 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
case VIR_TYPED_PARAM_BOOLEAN:
val[i].value.remote_typed_param_value_u.b = params[i].value.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+ if (val[i].value.remote_typed_param_value_u.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -739,8 +746,11 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
cleanup:
if (val) {
- for (i = 0; i < nparams; i++)
+ for (i = 0; i < nparams; i++) {
VIR_FREE(val[i].field);
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(val[i].value.remote_typed_param_value_u.s);
+ }
VIR_FREE(val);
}
return rv;
@@ -753,7 +763,7 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val,
int limit,
int *nparams)
{
- int i;
+ int i = 0;
int rv = -1;
virTypedParameterPtr params = NULL;
@@ -804,6 +814,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*args_params_val,
params[i].value.b =
args_params_val[i].value.remote_typed_param_value_u.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ params[i].value.s =
+ strdup(args_params_val[i].value.remote_typed_param_value_u.s);
+ if (params[i].value.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
virNetError(VIR_ERR_INTERNAL_ERROR, _("unknown parameter type:
%d"),
params[i].type);
@@ -814,8 +832,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*args_params_val,
rv = 0;
cleanup:
- if (rv < 0)
+ if (rv < 0) {
+ int j;
+ for (j = 0; j < i; ++j) {
+ if (params[j].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[j].value.s);
+ }
VIR_FREE(params);
+ }
return params;
}
@@ -1647,6 +1671,7 @@ success:
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
+ virTypedParameterArrayClear(params, nparams);
VIR_FREE(params);
if (dom)
virDomainFree(dom);
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index ea7fb24..2182c2c 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1244,8 +1244,11 @@ remoteFreeTypedParameters(remote_typed_param *args_params_val,
if (args_params_val == NULL)
return;
- for (i = 0; i < args_params_len; i++)
+ for (i = 0; i < args_params_len; i++) {
VIR_FREE(args_params_val[i].field);
+ if (args_params_val[i].value.type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(args_params_val[i].value.remote_typed_param_value_u.s);
+ }
VIR_FREE(args_params_val);
}
@@ -1294,6 +1297,13 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
case VIR_TYPED_PARAM_BOOLEAN:
val[i].value.remote_typed_param_value_u.b = params[i].value.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+ if (val[i].value.remote_typed_param_value_u.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -1318,7 +1328,7 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
virTypedParameterPtr params,
int *nparams)
{
- int i;
+ int i = 0;
int rv = -1;
/* Check the length of the returned list carefully. */
@@ -1365,6 +1375,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
params[i].value.b =
ret_params_val[i].value.remote_typed_param_value_u.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ params[i].value.s =
+ strdup(ret_params_val[i].value.remote_typed_param_value_u.s);
+ if (params[i].value.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -1375,6 +1393,12 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
rv = 0;
cleanup:
+ if (rv < 0) {
+ int j;
+ for (j = 0; j < i; j++)
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[i].value.s);
+ }
return rv;
}
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index a174af8..5ea1114 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -317,6 +317,8 @@ union remote_typed_param_value switch (int type) {
double d;
case VIR_TYPED_PARAM_BOOLEAN:
int b;
+ case VIR_TYPED_PARAM_STRING:
+ remote_nonnull_string s;
};
struct remote_typed_param {
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 12cedef..b460b77 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -6,6 +6,7 @@ enum {
VIR_TYPED_PARAM_ULLONG = 4,
VIR_TYPED_PARAM_DOUBLE = 5,
VIR_TYPED_PARAM_BOOLEAN = 6,
+ VIR_TYPED_PARAM_STRING = 7,
};
struct remote_nonnull_domain {
remote_nonnull_string name;
@@ -78,6 +79,7 @@ struct remote_typed_param_value {
uint64_t ul;
double d;
int b;
+ remote_nonnull_string s;
} remote_typed_param_value_u;
};
struct remote_typed_param {
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 56af258..b36ca69 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -439,7 +439,8 @@ elsif ($opt_b) {
"
$2,\n" .
"
&n$1)) == NULL)\n" .
" goto cleanup;\n");
- push(@free_list, " VIR_FREE(params);");
+ push(@free_list, " virTypedParameterArrayClear($1,
n$1);");
+ push(@free_list, " VIR_FREE($1);");
} elsif ($args_member =~ m/<\S+>;/ or $args_member =~ m/\[\S+\];/)
{
# just make all other array types fail
die "unhandled type for argument value: $args_member";
--
1.7.4.4
5:56 a.m.
New subject: [libvirt] [PATCHv5 2/3] API: remote support for VIR_TYPED_PARAM_STRING
On 11/01/2011 07:47 PM, Eric Blake wrote:
Send and receive string typed parameters across RPC.
* src/remote/remote_protocol.x (remote_typed_param_value): Add
another union value.
* daemon/remote.c (remoteSerializeTypedParameters)
(remoteDeserializeTypedParameters): Handle strings on rpc.
* src/remote/remote_driver.c (remoteFreeTypedParameters)
(remoteSerializeTypedParameters)
(remoteDeserializeTypedParameters): Likewise.
* src/rpc/gendispatch.pl: Properly clean up typed arrays.
* src/remote_protocol-structs: Update.
Based on an initial patch by Hu Tao, with feedback from
Daniel P. Berrange.
Signed-off-by: Eric Blake<eblake(a)redhat.com>
---
daemon/remote.c | 33 +++++++++++++++++++++++++++++----
src/remote/remote_driver.c | 28 ++++++++++++++++++++++++++--
src/remote/remote_protocol.x | 2 ++
src/remote_protocol-structs | 2 ++
src/rpc/gendispatch.pl | 3 ++-
5 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index bd0c3e3..e3a9775 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -701,7 +701,7 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
for (i = 0; i< nparams; ++i) {
/* remoteDispatchClientRequest will free this: */
- val[i].field = strdup (params[i].field);
+ val[i].field = strdup(params[i].field);
if (val[i].field == NULL) {
virReportOOMError();
goto cleanup;
@@ -726,6 +726,13 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
case VIR_TYPED_PARAM_BOOLEAN:
val[i].value.remote_typed_param_value_u.b = params[i].value.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+ if (val[i].value.remote_typed_param_value_u.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -739,8 +746,11 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
cleanup:
if (val) {
- for (i = 0; i< nparams; i++)
+ for (i = 0; i< nparams; i++) {
VIR_FREE(val[i].field);
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(val[i].value.remote_typed_param_value_u.s);
+ }
VIR_FREE(val);
}
return rv;
@@ -753,7 +763,7 @@ remoteDeserializeTypedParameters(remote_typed_param
*args_params_val,
int limit,
int *nparams)
{
- int i;
+ int i = 0;
int rv = -1;
virTypedParameterPtr params = NULL;
@@ -804,6 +814,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*args_params_val,
params[i].value.b =
args_params_val[i].value.remote_typed_param_value_u.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ params[i].value.s =
+ strdup(args_params_val[i].value.remote_typed_param_value_u.s);
+ if (params[i].value.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
virNetError(VIR_ERR_INTERNAL_ERROR, _("unknown parameter type:
%d"),
params[i].type);
@@ -814,8 +832,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*args_params_val,
rv = 0;
cleanup:
- if (rv< 0)
+ if (rv< 0) {
+ int j;
+ for (j = 0; j< i; ++j) {
+ if (params[j].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[j].value.s);
+ }
VIR_FREE(params);
+ }
return params;
}
@@ -1647,6 +1671,7 @@ success:
cleanup:
if (rv< 0)
virNetMessageSaveError(rerr);
+ virTypedParameterArrayClear(params, nparams);
VIR_FREE(params);
if (dom)
virDomainFree(dom);
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index ea7fb24..2182c2c 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -1244,8 +1244,11 @@ remoteFreeTypedParameters(remote_typed_param *args_params_val,
if (args_params_val == NULL)
return;
- for (i = 0; i< args_params_len; i++)
+ for (i = 0; i< args_params_len; i++) {
VIR_FREE(args_params_val[i].field);
+ if (args_params_val[i].value.type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(args_params_val[i].value.remote_typed_param_value_u.s);
+ }
VIR_FREE(args_params_val);
}
@@ -1294,6 +1297,13 @@ remoteSerializeTypedParameters(virTypedParameterPtr params,
case VIR_TYPED_PARAM_BOOLEAN:
val[i].value.remote_typed_param_value_u.b = params[i].value.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s);
+ if (val[i].value.remote_typed_param_value_u.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -1318,7 +1328,7 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
virTypedParameterPtr params,
int *nparams)
{
- int i;
+ int i = 0;
int rv = -1;
/* Check the length of the returned list carefully. */
@@ -1365,6 +1375,14 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
params[i].value.b =
ret_params_val[i].value.remote_typed_param_value_u.b;
break;
+ case VIR_TYPED_PARAM_STRING:
+ params[i].value.s =
+ strdup(ret_params_val[i].value.remote_typed_param_value_u.s);
+ if (params[i].value.s == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+ break;
default:
remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"),
params[i].type);
@@ -1375,6 +1393,12 @@ remoteDeserializeTypedParameters(remote_typed_param
*ret_params_val,
rv = 0;
cleanup:
+ if (rv< 0) {
+ int j;
+ for (j = 0; j< i; j++)
+ if (params[i].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[i].value.s);
Index has to be be 'j'!
+ if (params[j].type == VIR_TYPED_PARAM_STRING)
+ VIR_FREE(params[j].value.s);
+ }
return rv;
}
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index a174af8..5ea1114 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -317,6 +317,8 @@ union remote_typed_param_value switch (int type) {
double d;
case VIR_TYPED_PARAM_BOOLEAN:
int b;
+ case VIR_TYPED_PARAM_STRING:
+ remote_nonnull_string s;
};
struct remote_typed_param {
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 12cedef..b460b77 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -6,6 +6,7 @@ enum {
VIR_TYPED_PARAM_ULLONG = 4,
VIR_TYPED_PARAM_DOUBLE = 5,
VIR_TYPED_PARAM_BOOLEAN = 6,
+ VIR_TYPED_PARAM_STRING = 7,
};
struct remote_nonnull_domain {
remote_nonnull_string name;
@@ -78,6 +79,7 @@ struct remote_typed_param_value {
uint64_t ul;
double d;
int b;
+ remote_nonnull_string s;
} remote_typed_param_value_u;
};
struct remote_typed_param {
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 56af258..b36ca69 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -439,7 +439,8 @@ elsif ($opt_b) {
"
$2,\n" .
"&n$1)) == NULL)\n" .
" goto cleanup;\n");
- push(@free_list, " VIR_FREE(params);");
+ push(@free_list, " virTypedParameterArrayClear($1,
n$1);");
+ push(@free_list, " VIR_FREE($1);");
} elsif ($args_member =~ m/<\S+>;/ or $args_member =~
m/\[\S+\];/) {
# just make all other array types fail
die "unhandled type for argument value: $args_member";
Rest looks good to me. With nit fixed ACK.
6:47 p.m.
New subject: [libvirt] [PATCHv5 3/3] API: add trivial qemu support for VIR_TYPED_PARAM_STRING
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.
---
src/qemu/qemu_driver.c | 24 ++++++++++++++++++++----
1 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 37272e0..acb32d6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -920,6 +920,7 @@ qemudSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int
feature)
case VIR_DRV_FEATURE_MIGRATION_P2P:
case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
return 1;
default:
return 0;
@@ -6041,9 +6042,13 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,
bool isActive;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
- VIR_DOMAIN_AFFECT_CONFIG, -1);
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
+
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (vm == NULL) {
@@ -6336,10 +6341,14 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
bool isActive;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
- VIR_DOMAIN_AFFECT_CONFIG, -1);
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
+
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (vm == NULL) {
@@ -6883,10 +6892,14 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom,
int saved_nparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
- VIR_DOMAIN_AFFECT_CONFIG, -1);
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
+
if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) ==
(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) {
qemuReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -7142,7 +7155,10 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
long long wr_total_times, flush_req, flush_total_times, errs;
virTypedParameterPtr param;
- virCheckFlags(0, -1);
+ virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+ /* We don't return strings, and thus trivially support this flag. */
+ flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
--
1.7.4.4
6:12 a.m.
New subject: [libvirt] [PATCHv5 3/3] API: add trivial qemu support for VIR_TYPED_PARAM_STRING
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.
---
src/qemu/qemu_driver.c | 24 ++++++++++++++++++++----
1 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 37272e0..acb32d6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -920,6 +920,7 @@ qemudSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int
feature)
case VIR_DRV_FEATURE_MIGRATION_P2P:
case VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION:
case VIR_DRV_FEATURE_FD_PASSING:
+ case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
return 1;
default:
return 0;
@@ -6041,9 +6042,13 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,
bool isActive;
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?
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).
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
+
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (vm == NULL) {
@@ -6336,10 +6341,14 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom,
bool isActive;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
- VIR_DOMAIN_AFFECT_CONFIG, -1);
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
+
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
if (vm == NULL) {
@@ -6883,10 +6892,14 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom,
int saved_nparams = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
- VIR_DOMAIN_AFFECT_CONFIG, -1);
+ VIR_DOMAIN_AFFECT_CONFIG |
+ VIR_TYPED_PARAM_STRING_OKAY, -1);
qemuDriverLock(driver);
+ /* We don't return strings, and thus trivially support this flag. */
+ flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
+
if ((flags& (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) ==
(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) {
qemuReportError(VIR_ERR_INVALID_ARG, "%s",
@@ -7142,7 +7155,10 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
long long wr_total_times, flush_req, flush_total_times, errs;
virTypedParameterPtr param;
- virCheckFlags(0, -1);
+ virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+ /* We don't return strings, and thus trivially support this flag. */
+ flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
qemuDriverLock(driver);
vm = virDomainFindByUUID(&driver->domains, dom->uuid);
Rest looks
good. ACK
1:27 p.m.
New subject: [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
4768
days inactive
4770
days old
8 comments
2 participants
participants (2)
-
Eric Blake -
Stefan Berger