[libvirt] [PATCHv5 0/3] Add VIR_TYPED_PARAM_STRING
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
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@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
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@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.
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@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
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@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.
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
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
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
participants (2)
-
Eric Blake -
Stefan Berger