[libvirt] [PATCH 0/5] expose per-device blkio weight tuning, via string param
based on v6 by Eric. v6: https://www.redhat.com/archives/libvir-list/2011-November/msg00202.html Patches 1-3 are identical to v6 and have been already ACKed. posted here for your convenience. changes in patch 4: - fix some memory leaks - type of device weight changed to unsigned int - replace strncpy with virStrcpy changes in patch 5: - remove virCgroupGetBlkioDeviceWeight - live value of blkio.device_weight is read from vm's def - commas to seprate fields rather than commas between device and weight and semicolon between device-weight pairs - add virCgroupSetBlkioDeviceWeight to libvirt_private.syms 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 Hu Tao (2): blkiotune: add interface for blkiotune.device_weight blkiotune: add qemu support for blkiotune.device_weight daemon/remote.c | 88 +++++++++++----- docs/formatdomain.html.in | 29 +++++- docs/schemas/domaincommon.rng | 12 ++ include/libvirt/libvirt.h.in | 42 +++++++- src/conf/domain_conf.c | 138 +++++++++++++++++++++++- src/conf/domain_conf.h | 17 +++ src/libvirt.c | 92 ++++++++++++++--- src/libvirt_internal.h | 9 ++- src/libvirt_private.syms | 4 + src/qemu/qemu_cgroup.c | 22 ++++ src/qemu/qemu_driver.c | 238 +++++++++++++++++++++++++++++++++++++++-- src/remote/remote_driver.c | 28 +++++- src/remote/remote_protocol.x | 2 + src/remote_protocol-structs | 2 + src/rpc/gendispatch.pl | 3 +- src/util/cgroup.c | 20 ++++ src/util/cgroup.h | 3 + src/util/util.c | 14 +++ src/util/util.h | 2 + tools/virsh.c | 44 +++++++- tools/virsh.pod | 6 +- 21 files changed, 750 insertions(+), 65 deletions(-) -- 1.7.3.1
From: Eric Blake <eblake@redhat.com> 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 will be enforced at the RPC layer in the next patch, so that drivers need not worry about it in general. The one exception is that virDomainGetSchedulerParameters lacks a flags argument, so it must not return a string; drivers that forward that function on to virDomainGetSchedulerParametersFlags will have to pay attention to the flag. 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 | 32 ++++++++++++++- src/libvirt.c | 92 ++++++++++++++++++++++++++++++++++++------ src/libvirt_internal.h | 9 +++- src/libvirt_private.syms | 1 + src/util/util.c | 14 ++++++ src/util/util.h | 2 + 6 files changed, 134 insertions(+), 16 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa320b6..2ab89f5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -200,11 +200,14 @@ 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. */ VIR_DOMAIN_AFFECT_LIVE = 1 << 0, /* Affect running domain state. */ VIR_DOMAIN_AFFECT_CONFIG = 1 << 1, /* Affect persistent domain state. */ + /* 1 << 2 is reserved for virTypedParameterFlags */ } virDomainModificationImpact; /** @@ -489,10 +492,36 @@ 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 { + /* 1 << 0 is reserved for virDomainModificationImpact */ + /* 1 << 1 is reserved for virDomainModificationImpact */ + + /* 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 +549,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..1518ed2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3583,6 +3583,50 @@ 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; +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3621,6 +3665,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 +3691,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 +3742,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) { @@ -3717,7 +3767,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 +3799,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 +3825,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 +3867,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) { @@ -6410,7 +6466,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 +6512,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) { @@ -6505,15 +6564,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 +6629,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 +6728,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 +6778,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) { 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 959c224..9ecfa9d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2607,3 +2607,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 d8176a8..3295ce8 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -258,4 +258,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.3.1
On 11/08/2011 06:00 AM, Hu Tao wrote: Hi Hu, previously I had looked at v6. I suppose this is v7. I'll ACK what I had previously ack'ed in v6.
From: Eric Blake<eblake@redhat.com>
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 will be enforced at the RPC layer in the next patch, so that drivers need not worry about it in general. The one exception is that virDomainGetSchedulerParameters lacks a flags argument, so it must not return a string; drivers that forward that function on to virDomainGetSchedulerParametersFlags will have to pay attention to the flag. 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 | 32 ++++++++++++++- src/libvirt.c | 92 ++++++++++++++++++++++++++++++++++++------ src/libvirt_internal.h | 9 +++- src/libvirt_private.syms | 1 + src/util/util.c | 14 ++++++ src/util/util.h | 2 + 6 files changed, 134 insertions(+), 16 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa320b6..2ab89f5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -200,11 +200,14 @@ 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. */ VIR_DOMAIN_AFFECT_LIVE = 1<< 0, /* Affect running domain state. */ VIR_DOMAIN_AFFECT_CONFIG = 1<< 1, /* Affect persistent domain state. */ + /* 1<< 2 is reserved for virTypedParameterFlags */ } virDomainModificationImpact;
/** @@ -489,10 +492,36 @@ 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 { + /* 1<< 0 is reserved for virDomainModificationImpact */ + /* 1<< 1 is reserved for virDomainModificationImpact */ + + /* 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 +549,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..1518ed2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3583,6 +3583,50 @@ 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; +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3621,6 +3665,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 +3691,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 +3742,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) { @@ -3717,7 +3767,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 +3799,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 +3825,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 +3867,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) { @@ -6410,7 +6466,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 +6512,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) { @@ -6505,15 +6564,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 +6629,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 +6728,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 +6778,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) { 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 959c224..9ecfa9d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2607,3 +2607,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 d8176a8..3295ce8 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -258,4 +258,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__ */ ACK
On Tue, Nov 08, 2011 at 07:34:50AM -0500, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
Hi Hu,
previously I had looked at v6. I suppose this is v7. I'll ACK what I had previously ack'ed in v6.
Yes, this is v7. Thanks for ACK again. -- Thanks, Hu Tao
From: Eric Blake <eblake@redhat.com> Send and receive string typed parameters across RPC. This also completes the back-compat mentioned in the previous patch - the only time we have an older client talking to a newer server is if RPC is in use, so filtering out strings during RPC prevents returning an unknown type to the older client. * src/remote/remote_protocol.x (remote_typed_param_value): Add another union value. * daemon/remote.c (remoteDeserializeTypedParameters): Handle strings on rpc. (remoteSerializeTypedParameters): Likewise; plus filter out strings when replying to older clients. Adjust callers. * src/remote/remote_driver.c (remoteFreeTypedParameters) (remoteSerializeTypedParameters) (remoteDeserializeTypedParameters): Handle strings on rpc. * 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 | 88 +++++++++++++++++++++++++++++------------ 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, 94 insertions(+), 29 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index bd0c3e3..aa3f768 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -92,11 +92,6 @@ static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src); -static int -remoteSerializeTypedParameters(virTypedParameterPtr params, - int nparams, - remote_typed_param **ret_params_val, - u_int *ret_params_len); static virTypedParameterPtr remoteDeserializeTypedParameters(remote_typed_param *args_params_val, u_int args_params_len, @@ -682,14 +677,17 @@ cleanup: return rv; } -/* Helper to serialize typed parameters. */ +/* Helper to serialize typed parameters. This also filters out any string + * parameters that must not be returned to older clients. */ static int remoteSerializeTypedParameters(virTypedParameterPtr params, int nparams, remote_typed_param **ret_params_val, - u_int *ret_params_len) + u_int *ret_params_len, + unsigned int flags) { int i; + int j; int rv = -1; remote_typed_param *val; @@ -699,38 +697,53 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, goto cleanup; } - for (i = 0; i < nparams; ++i) { + for (i = 0, j = 0; i < nparams; ++i) { + if (!(flags & VIR_TYPED_PARAM_STRING_OKAY) && + params[i].type == VIR_TYPED_PARAM_STRING) { + --*ret_params_len; + continue; + } + /* remoteDispatchClientRequest will free this: */ - val[i].field = strdup (params[i].field); - if (val[i].field == NULL) { + val[j].field = strdup(params[i].field); + if (val[j].field == NULL) { virReportOOMError(); goto cleanup; } - val[i].value.type = params[i].type; - switch (params[i].type) { + val[j].value.type = params[i].type; + switch (params[j].type) { case VIR_TYPED_PARAM_INT: - val[i].value.remote_typed_param_value_u.i = params[i].value.i; + val[j].value.remote_typed_param_value_u.i = params[i].value.i; break; case VIR_TYPED_PARAM_UINT: - val[i].value.remote_typed_param_value_u.ui = params[i].value.ui; + val[j].value.remote_typed_param_value_u.ui = params[i].value.ui; break; case VIR_TYPED_PARAM_LLONG: - val[i].value.remote_typed_param_value_u.l = params[i].value.l; + val[j].value.remote_typed_param_value_u.l = params[i].value.l; break; case VIR_TYPED_PARAM_ULLONG: - val[i].value.remote_typed_param_value_u.ul = params[i].value.ul; + val[j].value.remote_typed_param_value_u.ul = params[i].value.ul; break; case VIR_TYPED_PARAM_DOUBLE: - val[i].value.remote_typed_param_value_u.d = params[i].value.d; + val[j].value.remote_typed_param_value_u.d = params[i].value.d; break; case VIR_TYPED_PARAM_BOOLEAN: - val[i].value.remote_typed_param_value_u.b = params[i].value.b; + val[j].value.remote_typed_param_value_u.b = params[i].value.b; + break; + case VIR_TYPED_PARAM_STRING: + val[j].value.remote_typed_param_value_u.s = + strdup(params[i].value.s); + if (val[j].value.remote_typed_param_value_u.s == NULL) { + virReportOOMError(); + goto cleanup; + } break; default: virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), params[i].type); goto cleanup; } + j++; } *ret_params_val = val; @@ -739,8 +752,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 +769,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 +820,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 +838,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; } @@ -854,7 +884,8 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, - &ret->params.params_len) < 0) + &ret->params.params_len, + 0) < 0) goto cleanup; rv = 0; @@ -908,7 +939,8 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, - &ret->params.params_len) < 0) + &ret->params.params_len, + args->flags) < 0) goto cleanup; rv = 0; @@ -1095,7 +1127,8 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialise the block stats. */ if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, - &ret->params.params_len) < 0) + &ret->params.params_len, + args->flags) < 0) goto cleanup; success: @@ -1575,7 +1608,8 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, - &ret->params.params_len) < 0) + &ret->params.params_len, + args->flags) < 0) goto cleanup; success: @@ -1638,7 +1672,8 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, - &ret->params.params_len) < 0) + &ret->params.params_len, + args->flags) < 0) goto cleanup; success: @@ -1647,6 +1682,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 f3b8ad5..94fd3e7 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[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"; -- 1.7.3.1
On 11/08/2011 06:00 AM, Hu Tao wrote:
From: Eric Blake<eblake@redhat.com>
Send and receive string typed parameters across RPC. This also completes the back-compat mentioned in the previous patch - the only time we have an older client talking to a newer server is if RPC is in use, so filtering out strings during RPC prevents returning an unknown type to the older client.
* src/remote/remote_protocol.x (remote_typed_param_value): Add another union value. * daemon/remote.c (remoteDeserializeTypedParameters): Handle strings on rpc. (remoteSerializeTypedParameters): Likewise; plus filter out strings when replying to older clients. Adjust callers. * src/remote/remote_driver.c (remoteFreeTypedParameters) (remoteSerializeTypedParameters) (remoteDeserializeTypedParameters): Handle strings on rpc. * 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 | 88 +++++++++++++++++++++++++++++------------ 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, 94 insertions(+), 29 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index bd0c3e3..aa3f768 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -92,11 +92,6 @@ static void make_nonnull_secret(remote_nonnull_secret *secret_dst, virSecretPtr static void make_nonnull_nwfilter(remote_nonnull_nwfilter *net_dst, virNWFilterPtr nwfilter_src); static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapshot_dst, virDomainSnapshotPtr snapshot_src);
-static int -remoteSerializeTypedParameters(virTypedParameterPtr params, - int nparams, - remote_typed_param **ret_params_val, - u_int *ret_params_len); static virTypedParameterPtr remoteDeserializeTypedParameters(remote_typed_param *args_params_val, u_int args_params_len, @@ -682,14 +677,17 @@ cleanup: return rv; }
-/* Helper to serialize typed parameters. */ +/* Helper to serialize typed parameters. This also filters out any string + * parameters that must not be returned to older clients. */ static int remoteSerializeTypedParameters(virTypedParameterPtr params, int nparams, remote_typed_param **ret_params_val, - u_int *ret_params_len) + u_int *ret_params_len, + unsigned int flags) { int i; + int j; int rv = -1; remote_typed_param *val;
@@ -699,38 +697,53 @@ remoteSerializeTypedParameters(virTypedParameterPtr params, goto cleanup; }
- for (i = 0; i< nparams; ++i) { + for (i = 0, j = 0; i< nparams; ++i) { + if (!(flags& VIR_TYPED_PARAM_STRING_OKAY)&& + params[i].type == VIR_TYPED_PARAM_STRING) { + --*ret_params_len; + continue; + } + /* remoteDispatchClientRequest will free this: */ - val[i].field = strdup (params[i].field); - if (val[i].field == NULL) { + val[j].field = strdup(params[i].field); + if (val[j].field == NULL) { virReportOOMError(); goto cleanup; } - val[i].value.type = params[i].type; - switch (params[i].type) { + val[j].value.type = params[i].type; + switch (params[j].type) { case VIR_TYPED_PARAM_INT: - val[i].value.remote_typed_param_value_u.i = params[i].value.i; + val[j].value.remote_typed_param_value_u.i = params[i].value.i; break; case VIR_TYPED_PARAM_UINT: - val[i].value.remote_typed_param_value_u.ui = params[i].value.ui; + val[j].value.remote_typed_param_value_u.ui = params[i].value.ui; break; case VIR_TYPED_PARAM_LLONG: - val[i].value.remote_typed_param_value_u.l = params[i].value.l; + val[j].value.remote_typed_param_value_u.l = params[i].value.l; break; case VIR_TYPED_PARAM_ULLONG: - val[i].value.remote_typed_param_value_u.ul = params[i].value.ul; + val[j].value.remote_typed_param_value_u.ul = params[i].value.ul; break; case VIR_TYPED_PARAM_DOUBLE: - val[i].value.remote_typed_param_value_u.d = params[i].value.d; + val[j].value.remote_typed_param_value_u.d = params[i].value.d; break; case VIR_TYPED_PARAM_BOOLEAN: - val[i].value.remote_typed_param_value_u.b = params[i].value.b; + val[j].value.remote_typed_param_value_u.b = params[i].value.b; + break; + case VIR_TYPED_PARAM_STRING: + val[j].value.remote_typed_param_value_u.s = + strdup(params[i].value.s); + if (val[j].value.remote_typed_param_value_u.s == NULL) { + virReportOOMError(); + goto cleanup; + } break; default: virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), params[i].type); goto cleanup; } + j++; }
*ret_params_val = val; @@ -739,8 +752,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 +769,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 +820,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 +838,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; }
@@ -854,7 +884,8 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS
if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, -&ret->params.params_len)< 0) +&ret->params.params_len, + 0)< 0) goto cleanup;
rv = 0; @@ -908,7 +939,8 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE
if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, -&ret->params.params_len)< 0) +&ret->params.params_len, + args->flags)< 0) goto cleanup;
rv = 0; @@ -1095,7 +1127,8 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, /* Serialise the block stats. */ if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, -&ret->params.params_len)< 0) +&ret->params.params_len, + args->flags)< 0) goto cleanup;
success: @@ -1575,7 +1608,8 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, -&ret->params.params_len)< 0) +&ret->params.params_len, + args->flags)< 0) goto cleanup;
success: @@ -1638,7 +1672,8 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, -&ret->params.params_len)< 0) +&ret->params.params_len, + args->flags)< 0) goto cleanup;
success: @@ -1647,6 +1682,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 f3b8ad5..94fd3e7 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[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"; ACK
From: Eric Blake <eblake@redhat.com> 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 118197a..c8a858e 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.3.1
On 11/08/2011 06:00 AM, Hu Tao wrote:
From: Eric Blake<eblake@redhat.com>
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 118197a..c8a858e 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); ACK
On 11/08/2011 05:36 AM, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
From: Eric Blake<eblake@redhat.com>
Qemu will be the first driver to make use of a typed string in the next round of additions. Separate out the trivial addition.
ACK
I've pushed 1-3 before running out of time today; I'll get to the rest as soon as I can (we have quite a backlog of unreviewed patches, and I'm trying to help some of them through - sorry if it feels slow to those waiting). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This adds per-device weights to <blkiotune>. Note that the cgroups implementation only supports weights per block device, and not per-file within the device; hence this option must be global to the domain definition rather than tied to individual <device>/<disk> entries: <domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ... This patch also adds a parameter --device-weight to virsh command blkiotune for setting/getting blkiotune.weight_device for any hypervisor that supports it. All <device> entries under <blkiotune> are concatenated into a single string attribute under virDomain{Get,Set}BlkioParameters, named "device_weight". Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++- docs/schemas/domaincommon.rng | 12 ++++ include/libvirt/libvirt.h.in | 10 +++ src/conf/domain_conf.c | 138 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 17 +++++ src/libvirt_private.syms | 2 + tools/virsh.c | 44 ++++++++++++-- tools/virsh.pod | 6 ++- 8 files changed, 246 insertions(+), 12 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..99c5add 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -505,6 +505,14 @@ ... <blkiotune> <weight>800</weight> + <device> + <path>/dev/sda</path> + <weight>1000</weight> + </device> + <device> + <path>/dev/sdb</path> + <weight>500</weight> + </device> </blkiotune> ... </domain> @@ -514,10 +522,25 @@ <dt><code>blkiotune</code></dt> <dd> The optional <code>blkiotune</code> element provides the ability to tune Blkio cgroup tunable parameters for the domain. If this is - omitted, it defaults to the OS provided defaults.</dd> + omitted, it defaults to the OS provided + defaults. <span class="since">Since 0.8.8</span></dd> <dt><code>weight</code></dt> - <dd> The optional <code>weight</code> element is the I/O weight of the - guest. The value should be in range [100, 1000].</dd> + <dd> The optional <code>weight</code> element is the overall I/O + weight of the guest. The value should be in the range [100, + 1000].</dd> + <dt><code>device</code></dt> + <dd>The domain may have multiple <code>device</code> elements + that further tune the weights for each host block device in + use by the domain. Note that + multiple <a href="#elementsDisks">guest disks</a> can share a + single host block device, if they are backed by files within + the same host file system, which is why this tuning parameter + is at the global domain level rather than associated with each + guest disk device. Each <code>device</code> element has two + mandatory sub-elements, <code>path</code> describing the + absolute path of the device, and <code>weight</code> giving + the relative weight of that device, in the range [100, + 1000]. <span class="since">Since 0.9.8</span></dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..347af1a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -328,6 +328,18 @@ <ref name="weight"/> </element> </optional> + <zeroOrMore> + <element name="device"> + <interleave> + <element name="path"> + <ref name="absFilePath"/> + </element> + <element name="weight"> + <ref name="weight"/> + </element> + </interleave> + </element> + </zeroOrMore> </element> </optional> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2ab89f5..0fd9302 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1236,6 +1236,16 @@ char * virDomainGetSchedulerType(virDomainPtr domain, #define VIR_DOMAIN_BLKIO_WEIGHT "weight" +/** + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight, as a string. The string is parsed as a + * series of /path/to/device,weight elements, separated by ';'. + */ + +#define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight" + /* Set Blkio tunables for the domain*/ int virDomainSetBlkioParameters(virDomainPtr domain, virTypedParameterPtr params, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a85f837..f0b324d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -603,6 +603,104 @@ VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE + +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices) +{ + int i; + + for (i = 0; i < ndevices; i++) + VIR_FREE(deviceWeights[i].path); +} + +/** + * virBlkioDeviceWeightToStr: + * + * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of per-device weights. + */ +#if defined(major) && defined(minor) +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i < ndevices; i++) { + if (stat(devices[i].path, &s) == -1) + return -1; + if ((s.st_mode & S_IFMT) != S_IFBLK) + return -1; + virBufferAsprintf(&buf, "%d:%d %d\n", + major(s.st_rdev), + minor(s.st_rdev), + devices[i].weight); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } + + *result = virBufferContentAndReset(&buf); + return 0; +} +#else +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1; +} +#endif + +/** + * virDomainBlkioDeviceWeightParseXML + * + * this function parses a XML node: + * + * <device> + * <path>/fully/qualified/device/path</path> + * <weight>weight</weight> + * </device> + * + * and fills a virBlkioDeviceWeight struct. + */ +static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) +{ + char *c; + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + dw->path = (char *)xmlNodeGetContent(node); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + if (virStrToLong_ui(c, NULL, 10, &dw->weight) < 0) { + VIR_FREE(c); + VIR_FREE(dw->path); + return -1; + } + VIR_FREE(c); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1272,6 +1370,10 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->emulator); VIR_FREE(def->description); + virBlkioDeviceWeightArrayClear(def->blkio.devices, + def->blkio.ndevices); + VIR_FREE(def->blkio.devices); + virDomainWatchdogDefFree(def->watchdog); virDomainMemballoonDefFree(def->memballoon); @@ -6711,6 +6813,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight) < 0) def->blkio.weight = 0; + n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes); + if (n > 0) { + if (VIR_ALLOC_N(def->blkio.devices, n) < 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i < n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], + &def->blkio.devices[i]) < 0) { + virBlkioDeviceWeightArrayClear(def->blkio.devices, i); + goto error; + } + } + def->blkio.ndevices = n; + VIR_FREE(nodes); + } + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit) < 0) @@ -10849,10 +10969,22 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon); /* add blkiotune only if there are any */ - if (def->blkio.weight) { + if (def->blkio.weight || def->blkio.devices) { virBufferAddLit(buf, " <blkiotune>\n"); - virBufferAsprintf(buf, " <weight>%u</weight>\n", - def->blkio.weight); + + if (def->blkio.weight) + virBufferAsprintf(buf, " <weight>%u</weight>\n", + def->blkio.weight); + + for (n = 0; n < def->blkio.ndevices; n++) { + virBufferAddLit(buf, " <device>\n"); + virBufferEscapeString(buf, " <path>%s</path>\n", + def->blkio.devices[n].path); + virBufferAsprintf(buf, " <weight>%u</weight>\n", + def->blkio.devices[n].weight); + virBufferAddLit(buf, " </device>\n"); + } + virBufferAddLit(buf, " </blkiotune>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5ebb441..7db2a11 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1361,6 +1361,20 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ }; +typedef struct _virBlkioDeviceWeight virBlkioDeviceWeight; +typedef virBlkioDeviceWeight *virBlkioDeviceWeightPtr; +struct _virBlkioDeviceWeight { + char *path; + unsigned int weight; +}; + +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices); +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1378,6 +1392,9 @@ struct _virDomainDef { struct { unsigned int weight; + + size_t ndevices; + virBlkioDeviceWeightPtr devices; } blkio; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2185294..f7d0fb2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,8 @@ virDomainAuditVcpu; # domain_conf.h +virBlkioDeviceWeightArrayClear; +virBlkioDeviceWeightToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/tools/virsh.c b/tools/virsh.c index 5544a41..fe87e9b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4648,6 +4648,8 @@ static const vshCmdOptDef opts_blkiotune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weights, in the form of /path/to/device,weight;...")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -4658,6 +4660,7 @@ static bool cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; + const char *device_weight = NULL; int weight = 0; int nparams = 0; int rv = 0; @@ -4702,6 +4705,16 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } } + rv = vshCommandOptString(cmd, "device-weight", &device_weight); + if (rv < 0) { + vshError(ctl, "%s", + _("Unable to parse string parameter")); + goto cleanup; + } + if (rv > 0) { + nparams++; + } + if (nparams == 0) { /* get the number of blkio parameters */ if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { @@ -4749,6 +4762,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "%-15s: %d\n", params[i].field, params[i].value.b); break; + case VIR_TYPED_PARAM_STRING: + vshPrint(ctl, "%-15s: %s\n", params[i].field, + params[i].value.s); + break; default: vshPrint(ctl, "unimplemented blkio parameter type\n"); } @@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight; + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - else - ret = true; + ret = false; + } } cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 775d302..a117e84 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1032,12 +1032,16 @@ value are kilobytes (i.e. blocks of 1024 bytes). Specifying -1 as a value for these limits is interpreted as unlimited. -=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] [[I<--config>] +=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] +[I<--device-weight> B<device-weights>] [[I<--config>] [I<--live>] | [I<--current>]] Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000]. +B<device-weights> is a single string listing one or more device/weight +pairs, in the format of /path/to/device,weight;/path/to/device,weight. + If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. -- 1.7.3.1
On 11/08/2011 06:00 AM, Hu Tao wrote:
This adds per-device weights to<blkiotune>. Note that the cgroups implementation only supports weights per block device, and not per-file within the device; hence this option must be global to the domain definition rather than tied to individual <device>/<disk> entries:
<domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ...
This patch also adds a parameter --device-weight to virsh command blkiotune for setting/getting blkiotune.weight_device for any hypervisor that supports it. All<device> entries under <blkiotune> are concatenated into a single string attribute under virDomain{Get,Set}BlkioParameters, named "device_weight".
Signed-off-by: Hu Tao<hutao@cn.fujitsu.com> Signed-off-by: Eric Blake<eblake@redhat.com> --- docs/formatdomain.html.in | 29 ++++++++- docs/schemas/domaincommon.rng | 12 ++++ include/libvirt/libvirt.h.in | 10 +++ src/conf/domain_conf.c | 138 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 17 +++++ src/libvirt_private.syms | 2 + tools/virsh.c | 44 ++++++++++++-- tools/virsh.pod | 6 ++- 8 files changed, 246 insertions(+), 12 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..99c5add 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -505,6 +505,14 @@ ... <blkiotune> <weight>800</weight> +<device> +<path>/dev/sda</path> +<weight>1000</weight> +</device> +<device> +<path>/dev/sdb</path> +<weight>500</weight> +</device> </blkiotune> ... </domain> @@ -514,10 +522,25 @@ <dt><code>blkiotune</code></dt> <dd> The optional<code>blkiotune</code> element provides the ability to tune Blkio cgroup tunable parameters for the domain. If this is - omitted, it defaults to the OS provided defaults.</dd> + omitted, it defaults to the OS provided + defaults.<span class="since">Since 0.8.8</span></dd> <dt><code>weight</code></dt> -<dd> The optional<code>weight</code> element is the I/O weight of the - guest. The value should be in range [100, 1000].</dd> +<dd> The optional<code>weight</code> element is the overall I/O + weight of the guest. The value should be in the range [100, + 1000].</dd> +<dt><code>device</code></dt> +<dd>The domain may have multiple<code>device</code> elements + that further tune the weights for each host block device in + use by the domain. Note that + multiple<a href="#elementsDisks">guest disks</a> can share a + single host block device, if they are backed by files within + the same host file system, which is why this tuning parameter + is at the global domain level rather than associated with each + guest disk device. Each<code>device</code> element has two + mandatory sub-elements,<code>path</code> describing the + absolute path of the device, and<code>weight</code> giving + the relative weight of that device, in the range [100, + 1000].<span class="since">Since 0.9.8</span></dd> </dl>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..347af1a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -328,6 +328,18 @@ <ref name="weight"/> </element> </optional> +<zeroOrMore> +<element name="device"> +<interleave> +<element name="path"> +<ref name="absFilePath"/> +</element> +<element name="weight"> +<ref name="weight"/> +</element> +</interleave> +</element> +</zeroOrMore> </element> </optional>
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2ab89f5..0fd9302 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1236,6 +1236,16 @@ char * virDomainGetSchedulerType(virDomainPtr domain,
#define VIR_DOMAIN_BLKIO_WEIGHT "weight"
+/** + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight, as a string. The string is parsed as a + * series of /path/to/device,weight elements, separated by ';'. + */ + +#define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight" + /* Set Blkio tunables for the domain*/ int virDomainSetBlkioParameters(virDomainPtr domain, virTypedParameterPtr params, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a85f837..f0b324d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -603,6 +603,104 @@ VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
+ +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices) +{ + int i; + + for (i = 0; i< ndevices; i++) + VIR_FREE(deviceWeights[i].path); +} + +/** + * virBlkioDeviceWeightToStr: + * + * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of per-device weights. + */ +#if defined(major)&& defined(minor) +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i< ndevices; i++) { + if (stat(devices[i].path,&s) == -1) + return -1; + if ((s.st_mode& S_IFMT) != S_IFBLK) + return -1; + virBufferAsprintf(&buf, "%d:%d %d\n", + major(s.st_rdev), + minor(s.st_rdev), + devices[i].weight); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } + + *result = virBufferContentAndReset(&buf); + return 0; +} +#else +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1; +} +#endif + +/** + * virDomainBlkioDeviceWeightParseXML + * + * this function parses a XML node: + * + *<device> + *<path>/fully/qualified/device/path</path> + *<weight>weight</weight> + *</device> + * + * and fills a virBlkioDeviceWeight struct. + */ +static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) +{ + char *c; + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + dw->path = (char *)xmlNodeGetContent(node); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + if (virStrToLong_ui(c, NULL, 10,&dw->weight)< 0) { + VIR_FREE(c); + VIR_FREE(dw->path); + return -1; + } + VIR_FREE(c); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1272,6 +1370,10 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->emulator); VIR_FREE(def->description);
+ virBlkioDeviceWeightArrayClear(def->blkio.devices, + def->blkio.ndevices); + VIR_FREE(def->blkio.devices); + virDomainWatchdogDefFree(def->watchdog);
virDomainMemballoonDefFree(def->memballoon); @@ -6711,6 +6813,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight)< 0) def->blkio.weight = 0;
+ n = virXPathNodeSet("./blkiotune/device", ctxt,&nodes); + if (n> 0) { + if (VIR_ALLOC_N(def->blkio.devices, n)< 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i< n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], +&def->blkio.devices[i])< 0) { + virBlkioDeviceWeightArrayClear(def->blkio.devices, i); + goto error; + } + } + def->blkio.ndevices = n; + VIR_FREE(nodes); + } + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit)< 0) @@ -10849,10 +10969,22 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon);
/* add blkiotune only if there are any */ - if (def->blkio.weight) { + if (def->blkio.weight || def->blkio.devices) { virBufferAddLit(buf, "<blkiotune>\n"); - virBufferAsprintf(buf, "<weight>%u</weight>\n", - def->blkio.weight); + + if (def->blkio.weight) + virBufferAsprintf(buf, "<weight>%u</weight>\n", + def->blkio.weight); + + for (n = 0; n< def->blkio.ndevices; n++) { + virBufferAddLit(buf, "<device>\n"); + virBufferEscapeString(buf, "<path>%s</path>\n", + def->blkio.devices[n].path); + virBufferAsprintf(buf, "<weight>%u</weight>\n", + def->blkio.devices[n].weight); + virBufferAddLit(buf, "</device>\n"); + } + virBufferAddLit(buf, "</blkiotune>\n"); }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5ebb441..7db2a11 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1361,6 +1361,20 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ };
+typedef struct _virBlkioDeviceWeight virBlkioDeviceWeight; +typedef virBlkioDeviceWeight *virBlkioDeviceWeightPtr; +struct _virBlkioDeviceWeight { + char *path; + unsigned int weight; +}; + +void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices); +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1378,6 +1392,9 @@ struct _virDomainDef {
struct { unsigned int weight; + + size_t ndevices; + virBlkioDeviceWeightPtr devices; } blkio;
struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2185294..f7d0fb2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,8 @@ virDomainAuditVcpu;
# domain_conf.h +virBlkioDeviceWeightArrayClear; +virBlkioDeviceWeightToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/tools/virsh.c b/tools/virsh.c index 5544a41..fe87e9b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4648,6 +4648,8 @@ static const vshCmdOptDef opts_blkiotune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weights, in the form of /path/to/device,weight;...")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -4658,6 +4660,7 @@ static bool cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; + const char *device_weight = NULL; int weight = 0; int nparams = 0; int rv = 0; @@ -4702,6 +4705,16 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } }
+ rv = vshCommandOptString(cmd, "device-weight",&device_weight); + if (rv< 0) { + vshError(ctl, "%s", + _("Unable to parse string parameter")); + goto cleanup; + } + if (rv> 0) { + nparams++; + } + if (nparams == 0) { /* get the number of blkio parameters */ if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) { @@ -4749,6 +4762,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "%-15s: %d\n", params[i].field, params[i].value.b); break; + case VIR_TYPED_PARAM_STRING: + vshPrint(ctl, "%-15s: %s\n", params[i].field, + params[i].value.s); + break; default: vshPrint(ctl, "unimplemented blkio parameter type\n"); } @@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight; + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - else - ret = true; + ret = false; + } }
cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 775d302..a117e84 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1032,12 +1032,16 @@ value are kilobytes (i.e. blocks of 1024 bytes).
Specifying -1 as a value for these limits is interpreted as unlimited.
-=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] [[I<--config>] +=item B<blkiotune> I<domain-id> [I<--weight> B<weight>] +[I<--device-weight> B<device-weights>] [[I<--config>] [I<--live>] | [I<--current>]]
Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000].
+B<device-weights> is a single string listing one or more device/weight +pairs, in the format of /path/to/device,weight;/path/to/device,weight. + If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. ACK
On 11/08/2011 05:43 AM, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
This adds per-device weights to<blkiotune>. Note that the cgroups implementation only supports weights per block device, and not per-file within the device; hence this option must be global to the domain definition rather than tied to individual <device>/<disk> entries:
<domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ...
This patch also adds a parameter --device-weight to virsh command blkiotune for setting/getting blkiotune.weight_device for any hypervisor that supports it. All<device> entries under <blkiotune> are concatenated into a single string attribute under virDomain{Get,Set}BlkioParameters, named "device_weight".
+/** + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight, as a string. The string is parsed as a + * series of /path/to/device,weight elements, separated by ';'.
It looks like the rest of the code swapped over to "separated by ','", so I made that tweak.
+ * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of per-device weights. + */ +#if defined(major)&& defined(minor) +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i< ndevices; i++) { + if (stat(devices[i].path,&s) == -1) + return -1; + if ((s.st_mode& S_IFMT) != S_IFBLK) + return -1; + virBufferAsprintf(&buf, "%d:%d %d\n", + major(s.st_rdev), + minor(s.st_rdev), + devices[i].weight); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } + + *result = virBufferContentAndReset(&buf); + return 0;
You fail for more than one reason, but don't output a failure log message, which means the caller has to do it and can't guess why things failed (OOM? not a block device?). I don't see any callers in this patch, so I checked 5/5; there, both callers just reported an internal error stating the string could not be parsed. But internal error for a user-visible message is rather harsh, so I think this is worth improving.
+} +#else +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1;
And if we improve the error reporting above, then we also have to report an error here.
+static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) +{ + char *c; + xmlNodePtr node; + + if (!dw) + return -1;
Dead check - this is a static function, and we know the caller gives us valid inputs.
+ node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + dw->path = (char *)xmlNodeGetContent(node);
Memory leak; if the user (mistakenly) has more than one <path> subelement, then you are blindly overwriting dw->path on the second instance of path.
+ } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + if (virStrToLong_ui(c, NULL, 10,&dw->weight)< 0) { + VIR_FREE(c); + VIR_FREE(dw->path); + return -1;
Missing error reporting - either we must report the error here or in the caller (I elected for here, to match precedence with other functions used by the caller).
+ } + VIR_FREE(c); + } + } + node = node->next; + }
You never validated that path was a mandatory element.
@@ -6711,6 +6813,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight)< 0) def->blkio.weight = 0;
+ n = virXPathNodeSet("./blkiotune/device", ctxt,&nodes); + if (n> 0) {
Missing an OOM memory check.
+ if (VIR_ALLOC_N(def->blkio.devices, n)< 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i< n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], +&def->blkio.devices[i])< 0) { + virBlkioDeviceWeightArrayClear(def->blkio.devices, i);
Redundant, if you had been updating def->blkio.ndevices as you go, for consistency with some of the other clients of virXPathNodeSet in this method.
+++ b/tools/virsh.c @@ -4648,6 +4648,8 @@ static const vshCmdOptDef opts_blkiotune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weights, in the form of /path/to/device,weight;...")},
Another place where we want to use ',', not ';'.
@@ -4749,6 +4762,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "%-15s: %d\n", params[i].field, params[i].value.b); break; + case VIR_TYPED_PARAM_STRING: + vshPrint(ctl, "%-15s: %s\n", params[i].field, + params[i].value.s); + break; default: vshPrint(ctl, "unimplemented blkio parameter type\n"); }
Memory leak - we aren't calling virTypedParameterArrayClear to free up any returned strings that we just printed. But we can't blindly call it in the cleanup: label of the function, because...
@@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight;
...here we are not storing malloc'd memory there in the first place. Using a judicious strdup simplifies the code.
+ temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false;
ret is already false, we can skip this assignment.
+ goto cleanup; + } + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) {
Pre-existing, but as long as we are touching this code, it's better to check for < 0 instead of != 0 for errors on libvirt calls.
+B<device-weights> is a single string listing one or more device/weight +pairs, in the format of /path/to/device,weight;/path/to/device,weight.
Another comma.
+ If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. ACK
I won't push this until I've tweaked and tested patch 5/5, but here's what I plan on squashing in provided testing goes well. diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 0fd9302..ff4f51b 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -1241,7 +1241,7 @@ char * virDomainGetSchedulerType(virDomainPtr domain, * * Macro for the blkio tunable weight_device: it represents the * per-device weight, as a string. The string is parsed as a - * series of /path/to/device,weight elements, separated by ';'. + * series of /path/to/device,weight elements, separated by ','. */ #define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight" diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index e4ae64c..8ac8d6f 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -604,8 +604,9 @@ VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE -void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, - int ndevices) +void +virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices) { int i; @@ -618,22 +619,26 @@ void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, * * This function returns a string representing device weights that is * suitable for writing to /cgroup/blkio/blkio.weight_device, given - * a list of per-device weights. + * a list of per-device weights, or reports an error on failure. */ #if defined(major) && defined(minor) -int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, - int ndevices, - char **result) +int +virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) { int i; struct stat s; virBuffer buf = VIR_BUFFER_INITIALIZER; for (i = 0; i < ndevices; i++) { - if (stat(devices[i].path, &s) == -1) - return -1; - if ((s.st_mode & S_IFMT) != S_IFBLK) + if (stat(devices[i].path, &s) == -1 || + (s.st_mode & S_IFMT) != S_IFBLK) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("path '%s' must be a block device"), + devices[i].path); return -1; + } virBufferAsprintf(&buf, "%d:%d %d\n", major(s.st_rdev), minor(s.st_rdev), @@ -641,6 +646,7 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, } if (virBufferError(&buf)) { + virReportOOMError(); virBufferFreeAndReset(&buf); return -1; } @@ -649,10 +655,13 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, return 0; } #else -int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, - int ndevices ATTRIBUTE_UNUSED, - char **result ATTRIBUTE_UNUSED) +int +virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this binary lacks per-device weight support")); return -1; } #endif @@ -669,23 +678,24 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, * * and fills a virBlkioDeviceWeight struct. */ -static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, - virBlkioDeviceWeightPtr dw) +static int +virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) { char *c; xmlNodePtr node; - if (!dw) - return -1; - node = root->children; while (node) { if (node->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(node->name, BAD_CAST "path")) { + if (xmlStrEqual(node->name, BAD_CAST "path") && !dw->path) { dw->path = (char *)xmlNodeGetContent(node); } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { c = (char *)xmlNodeGetContent(node); if (virStrToLong_ui(c, NULL, 10, &dw->weight) < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse weight %s"), + c); VIR_FREE(c); VIR_FREE(dw->path); return -1; @@ -695,6 +705,11 @@ static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, } node = node->next; } + if (!dw->path) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing per-device path")); + return -1; + } return 0; } @@ -6813,23 +6828,21 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight) < 0) def->blkio.weight = 0; - n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes); - if (n > 0) { - if (VIR_ALLOC_N(def->blkio.devices, n) < 0) { - virReportOOMError(); - goto error; - } + if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract blkiotune nodes")); + goto error; + } + if (n && VIR_ALLOC_N(def->blkio.devices, n) < 0) + goto no_memory; - for (i = 0; i < n; i++) { - if (virDomainBlkioDeviceWeightParseXML(nodes[i], - &def->blkio.devices[i]) < 0) { - virBlkioDeviceWeightArrayClear(def->blkio.devices, i); - goto error; - } - } - def->blkio.ndevices = n; - VIR_FREE(nodes); + for (i = 0; i < n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], + &def->blkio.devices[i]) < 0) + goto error; + def->blkio.ndevices++; } + VIR_FREE(nodes); /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, diff --git i/tools/virsh.c w/tools/virsh.c index 4060e3d..30504be 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -4649,7 +4649,7 @@ static const vshCmdOptDef opts_blkiotune[] = { {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, - N_("per-device IO Weights, in the form of /path/to/device,weight;...")}, + N_("per-device IO Weights, in the form of /path/to/device,weight,...")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -4770,8 +4770,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "unimplemented blkio parameter type\n"); } } - - ret = true; } else { /* set the blkio parameters */ params = vshCalloc(ctl, nparams, sizeof(*params)); @@ -4783,34 +4781,29 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (weight) { temp->value.ui = weight; if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field))) { - virTypedParameterArrayClear(params, nparams); - ret = false; + sizeof(temp->field))) goto cleanup; - } - weight = 0; } if (device_weight) { - /* Safe to cast away const here. */ - temp->value.s = (char *)device_weight; + temp->value.s = vshStrdup(ctl, device_weight); temp->type = VIR_TYPED_PARAM_STRING; if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, - sizeof(temp->field))) { - virTypedParameterArrayClear(params, nparams); - ret = false; + sizeof(temp->field))) goto cleanup; - } } } - ret = true; - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + + if (virDomainSetBlkioParameters(dom, params, nparams, flags) < 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - ret = false; + goto cleanup; } } + ret = true; + cleanup: + virTypedParameterArrayClear(params, nparams); VIR_FREE(params); virDomainFree(dom); return ret; diff --git i/tools/virsh.pod w/tools/virsh.pod index a117e84..70e299f 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -1040,7 +1040,7 @@ Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000]. B<device-weights> is a single string listing one or more device/weight -pairs, in the format of /path/to/device,weight;/path/to/device,weight. +pairs, in the format of /path/to/device,weight,/path/to/device,weight. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Implement setting/getting per-device blkio weights in qemu, using the cgroups blkio.weight_device tunable. --- src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 216 ++++++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 20 ++++ src/util/cgroup.h | 3 + 5 files changed, 257 insertions(+), 5 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f7d0fb2..b32d5de 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -89,6 +89,7 @@ virCgroupKillRecursive; virCgroupMounted; virCgroupPathOfController; virCgroupRemove; +virCgroupSetBlkioDeviceWeight; virCgroupSetBlkioWeight; virCgroupSetCpuShares; virCgroupSetCpuCfsPeriod; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..d397578 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver, } } + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + char *tmp; + if (virBlkioDeviceWeightToStr(vm->def->blkio.devices, + vm->def->blkio.ndevices, + &tmp) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set io device weight for domain %s"), + vm->def->name); + goto cleanup; + } + if (tmp) { + rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to set io device weight for domain %s"), + vm->def->name); + goto cleanup; + } + } + } + if (vm->def->mem.hard_limit != 0 || vm->def->mem.soft_limit != 0 || vm->def->mem.swap_hard_limit != 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8a858e..40568d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,7 +112,7 @@ # define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ #endif -#define QEMU_NB_BLKIO_PARAM 1 +#define QEMU_NB_BLKIO_PARAM 2 static void processWatchdogEvent(void *data, void *opaque); @@ -5888,6 +5888,86 @@ cleanup: return ret; } +/* deviceWeightStr in the form of /device/path,weight,/device/path,weight + * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800 + */ +static int +parseBlkioWeightDeviceStr(char *deviceWeightStr, + virBlkioDeviceWeightPtr *dw, int *size) +{ + char *temp; + int ndevices = 0; + int nsep = 0; + int i; + virBlkioDeviceWeightPtr result = NULL; + + temp = deviceWeightStr; + while (temp) { + temp = strchr(temp, ','); + if (temp) { + temp++; + nsep++; + } + } + + /* a valid string must have even number of fields */ + if (!(nsep & 1)) + goto error; + + ndevices = (nsep + 1) / 2; + + if (VIR_ALLOC_N(result, ndevices) < 0) { + virReportOOMError(); + return -1; + } + + i = 0; + temp = deviceWeightStr; + while (temp && i < ndevices) { + char *p = temp; + + /* device path */ + + p = strchr(p, ','); + if (!p) + goto error; + + result[i].path = strndup(temp, p - temp); + if (!result[i].path) { + virReportOOMError(); + goto cleanup; + } + + /* weight */ + temp = p + 1; + + if (virStrToLong_ui(temp, &p, 10, &result[i].weight) < 0) + goto error; + + i++; + if (*p == '\0') + break; + else if (*p != ',') + goto error; + temp = p + 1; + } + if (i != ndevices) + goto error; + + *dw = result; + *size = i; + + return 0; + +error: + qemuReportError(VIR_ERR_INVALID_ARG, + _("unable to parse %s"), deviceWeightStr); +cleanup: + virBlkioDeviceWeightArrayClear(result, ndevices); + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5954,10 +6034,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { + int rc; virTypedParameterPtr param = ¶ms[i]; if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - int rc; if (param->type != VIR_TYPED_PARAM_UINT) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid type for blkio weight tunable, expected a 'unsigned int'")); @@ -5978,6 +6058,53 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, _("unable to set blkio weight tunable")); ret = -1; } + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + int ndevices; + virBlkioDeviceWeightPtr devices = NULL; + char *tmp; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for device_weight tunable, expected a 'char *'")); + ret = -1; + continue; + } + + if (parseBlkioWeightDeviceStr(params[i].value.s, + &devices, + &ndevices) < 0) { + ret = -1; + continue; + } + if (virBlkioDeviceWeightToStr(devices, + ndevices, + &tmp) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set blkio weight_device tunable")); + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + ret = -1; + continue; + } + if (tmp) { + rc = virCgroupSetBlkioDeviceWeight(group, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + continue; + } + virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, + vm->def->blkio.ndevices); + VIR_FREE(vm->def->blkio.devices); + vm->def->blkio.devices = devices; + vm->def->blkio.ndevices = ndevices; + } else { + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6007,9 +6134,24 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } persistentDef->blkio.weight = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + virBlkioDeviceWeightPtr devices = NULL; + int ndevices; + if (parseBlkioWeightDeviceStr(params[i].value.s, + &devices, + &ndevices) < 0) { + ret = -1; + continue; + } + virBlkioDeviceWeightArrayClear(persistentDef->blkio.devices, + persistentDef->blkio.ndevices); + VIR_FREE(persistentDef->blkio.devices); + persistentDef->blkio.devices = devices; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, - _("Parameter `%s' not supported"), param->field); + _("Parameter `%s' not supported"), + param->field); ret = -1; } } @@ -6032,7 +6174,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; - int i; + int i, j; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -6046,7 +6188,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, VIR_TYPED_PARAM_STRING_OKAY, -1); qemuDriverLock(driver); - /* We don't return strings, and thus trivially support this flag. */ + /* We blindly return a string, and let libvirt.c do the filtering + * on behalf of older clients that can't parse it. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6125,6 +6268,37 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkiotune.device_weight */ + if (vm->def->blkio.ndevices > 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j < vm->def->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + vm->def->blkio.devices[j].path, + vm->def->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break; default: break; @@ -6149,6 +6323,38 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break; + case 1: /* blkiotune.device_weight */ + if (persistentDef->blkio.ndevices > 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j < persistentDef->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + persistentDef->blkio.devices[j].path, + persistentDef->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break; + default: break; /* should not hit here */ diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c8d1f33..b510640 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -982,6 +982,26 @@ int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) } /** + * virCgroupSetBlkioDeviceWeight: + * + * @group: The cgroup to change io device weight device for + * @device_weight: The device weight for this cgroup + * + * device_weight is treated as a write-only parameter, so + * there isn't a getter counterpart. + * + * Returns: 0 on success + */ +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *device_weight) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + device_weight); +} + +/** * virCgroupSetMemory: * * @group: The cgroup to change memory for diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d190bb3..9972671 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -55,6 +55,9 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight); +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *device_weight); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); -- 1.7.3.1
On 11/08/2011 06:00 AM, Hu Tao wrote:
Implement setting/getting per-device blkio weights in qemu, using the cgroups blkio.weight_device tunable.
--- src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 216 ++++++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 20 ++++ src/util/cgroup.h | 3 + 5 files changed, 257 insertions(+), 5 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f7d0fb2..b32d5de 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -89,6 +89,7 @@ virCgroupKillRecursive; virCgroupMounted; virCgroupPathOfController; virCgroupRemove; +virCgroupSetBlkioDeviceWeight; virCgroupSetBlkioWeight; virCgroupSetCpuShares; virCgroupSetCpuCfsPeriod; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..d397578 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver, } }
+ if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + char *tmp; + if (virBlkioDeviceWeightToStr(vm->def->blkio.devices, + vm->def->blkio.ndevices, +&tmp)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set io device weight for domain %s"), + vm->def->name); The ToStr function doesn't report an OOM error. So this is ok. I wonder whether that function should report an OOM error, though? Eric? + goto cleanup; + } + if (tmp) { I think you can remove the if branch. + rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to set io device weight for domain %s"), + vm->def->name); + goto cleanup; + } + } + } + if (vm->def->mem.hard_limit != 0 || vm->def->mem.soft_limit != 0 || vm->def->mem.swap_hard_limit != 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8a858e..40568d0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -112,7 +112,7 @@ # define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ #endif
-#define QEMU_NB_BLKIO_PARAM 1 +#define QEMU_NB_BLKIO_PARAM 2
static void processWatchdogEvent(void *data, void *opaque);
@@ -5888,6 +5888,86 @@ cleanup: return ret; }
+/* deviceWeightStr in the form of /device/path,weight,/device/path,weight + * for example, /dev/disk/by-path/pci-0000:00:1f.2-scsi-0:0:0:0,800 + */ +static int +parseBlkioWeightDeviceStr(char *deviceWeightStr, + virBlkioDeviceWeightPtr *dw, int *size) +{ + char *temp; + int ndevices = 0; + int nsep = 0; + int i; + virBlkioDeviceWeightPtr result = NULL; + + temp = deviceWeightStr; + while (temp) { + temp = strchr(temp, ','); + if (temp) { + temp++; + nsep++; + } + } + + /* a valid string must have even number of fields */ ...and an odd number of commas. + if (!(nsep& 1)) + goto error; + + ndevices = (nsep + 1) / 2; + + if (VIR_ALLOC_N(result, ndevices)< 0) { + virReportOOMError(); + return -1; + } + + i = 0; + temp = deviceWeightStr; + while (temp&& i< ndevices) { + char *p = temp; + + /* device path */ + + p = strchr(p, ','); + if (!p) + goto error; + + result[i].path = strndup(temp, p - temp); + if (!result[i].path) { + virReportOOMError(); + goto cleanup; + } + + /* weight */ + temp = p + 1; + + if (virStrToLong_ui(temp,&p, 10,&result[i].weight)< 0) + goto error; + + i++; + if (*p == '\0') + break; + else if (*p != ',') + goto error; + temp = p + 1; + } + if (i != ndevices) + goto error; + + *dw = result; + *size = i; + + return 0; + +error: + qemuReportError(VIR_ERR_INVALID_ARG, + _("unable to parse %s"), deviceWeightStr); +cleanup: + virBlkioDeviceWeightArrayClear(result, ndevices); + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5954,10 +6034,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags& VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i< nparams; i++) { + int rc; virTypedParameterPtr param =¶ms[i];
if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT)) { - int rc; if (param->type != VIR_TYPED_PARAM_UINT) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid type for blkio weight tunable, expected a 'unsigned int'")); @@ -5978,6 +6058,53 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, _("unable to set blkio weight tunable")); ret = -1; } + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + int ndevices; + virBlkioDeviceWeightPtr devices = NULL; + char *tmp; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for device_weight tunable, expected a 'char *'")); + ret = -1; + continue; + } + + if (parseBlkioWeightDeviceStr(params[i].value.s, +&devices, +&ndevices)< 0) { + ret = -1; + continue; + } + if (virBlkioDeviceWeightToStr(devices, + ndevices, +&tmp)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set blkio weight_device tunable")); + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + ret = -1; + continue; + } + if (tmp) { + rc = virCgroupSetBlkioDeviceWeight(group, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); indentation + ret = -1; + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + continue; + } + virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, + vm->def->blkio.ndevices); + VIR_FREE(vm->def->blkio.devices); + vm->def->blkio.devices = devices; + vm->def->blkio.ndevices = ndevices; + } else { + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6007,9 +6134,24 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, }
persistentDef->blkio.weight = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + virBlkioDeviceWeightPtr devices = NULL; + int ndevices; + if (parseBlkioWeightDeviceStr(params[i].value.s, +&devices, +&ndevices)< 0) { + ret = -1; + continue; + } + virBlkioDeviceWeightArrayClear(persistentDef->blkio.devices, + persistentDef->blkio.ndevices); + VIR_FREE(persistentDef->blkio.devices); + persistentDef->blkio.devices = devices; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, - _("Parameter `%s' not supported"), param->field); + _("Parameter `%s' not supported"), + param->field); ret = -1; } } @@ -6032,7 +6174,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; - int i; + int i, j; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; @@ -6046,7 +6188,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, VIR_TYPED_PARAM_STRING_OKAY, -1); qemuDriverLock(driver);
- /* We don't return strings, and thus trivially support this flag. */ + /* We blindly return a string, and let libvirt.c do the filtering + * on behalf of older clients that can't parse it. */ flags&= ~VIR_TYPED_PARAM_STRING_OKAY;
vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6125,6 +6268,37 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkiotune.device_weight */ + if (vm->def->blkio.ndevices> 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j< vm->def->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + vm->def->blkio.devices[j].path, + vm->def->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { virBufferFreeAndReset(&buf) + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), .. is too long + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break;
default: break; @@ -6149,6 +6323,38 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break;
+ case 1: /* blkiotune.device_weight */ + if (persistentDef->blkio.ndevices> 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j< persistentDef->blkio.ndevices; j++) { + if (j) + virBufferAddChar(&buf, ','); + virBufferAsprintf(&buf, "%s,%u", + persistentDef->blkio.devices[j].path, + persistentDef->blkio.devices[j].weight); + } + if (virBufferError(&buf)) { virBufferFreeAndReset(&buf); + virReportOOMError(); + goto cleanup; + } + param->value.s = virBufferContentAndReset(&buf); + } else { + param->value.s = strdup(""); + if (!param->value.s) { + virReportOOMError(); + goto cleanup; + } + } + param->type = VIR_TYPED_PARAM_STRING; + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_DEVICE_WEIGHT); + goto cleanup; + } + break; + default: break; /* should not hit here */ diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c8d1f33..b510640 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -982,6 +982,26 @@ int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) }
/** + * virCgroupSetBlkioDeviceWeight: + * + * @group: The cgroup to change io device weight device for + * @device_weight: The device weight for this cgroup + * + * device_weight is treated as a write-only parameter, so + * there isn't a getter counterpart. + * + * Returns: 0 on success + */ +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *device_weight) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + device_weight); +} + +/** * virCgroupSetMemory: * * @group: The cgroup to change memory for diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d190bb3..9972671 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -55,6 +55,9 @@ int virCgroupAddTask(virCgroupPtr group, pid_t pid); int virCgroupSetBlkioWeight(virCgroupPtr group, unsigned int weight); int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight);
+int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *device_weight); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb);
ACK with those nits fixed.
On 11/08/2011 06:05 AM, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
Implement setting/getting per-device blkio weights in qemu, using the cgroups blkio.weight_device tunable.
--- src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 216 ++++++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 20 ++++ src/util/cgroup.h | 3 + 5 files changed, 257 insertions(+), 5 deletions(-)
In addition to Stefan's comments, which I fixed as recommended, I had some additional comments. In particular, qemu's set blkio parameters had a pre-existing bug, where using 'virsh blkiotune --live --persistent' failed to update persistent state, which I noticed in testing this (a simple matter of removing a bogus 'else').
--- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -312,6 +312,28 @@ int qemuSetupCgroup(struct qemud_driver *driver, } }
+ if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + char *tmp; + if (virBlkioDeviceWeightToStr(vm->def->blkio.devices, + vm->def->blkio.ndevices, +&tmp)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set io device weight for domain %s"), + vm->def->name); The ToStr function doesn't report an OOM error. So this is ok. I wonder whether that function should report an OOM error, though? Eric?
I fixed that in my review of 4/5 to have ToStr always report error, so no need to report error here.
+ goto cleanup; + } + if (tmp) { I think you can remove the if branch.
Not entirely true. virBlkioDeviceWeightToStr sets tmp to NULL and returns 0 if vm->def->blkio.ndevices is 0. But in that case, why even bother going into this entire block of code?
@@ -5978,6 +6058,53 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, _("unable to set blkio weight tunable")); ret = -1; } + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT)) { + int ndevices; + virBlkioDeviceWeightPtr devices = NULL; + char *tmp; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for device_weight tunable, expected a 'char *'")); + ret = -1; + continue; + } + + if (parseBlkioWeightDeviceStr(params[i].value.s, +&devices, +&ndevices)< 0) { + ret = -1; + continue; + } + if (virBlkioDeviceWeightToStr(devices, + ndevices, +&tmp)< 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set blkio weight_device tunable")); + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + ret = -1; + continue; + } + if (tmp) {
Useless conditional; here we know tmp is non-NULL because virBlkioDeviceWeightToStr succeeded.
ACK with those nits fixed.
In my testing, I ran into a weird issue. From the shell: printf '8:0 500\n8:16 500\n' > /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device succeeded at altering the cgroup contents. But: /usr/bin/printf '8:0 500\n8:16 500\n' > /cgroup/blkio/libvirt/qemu/dom/blkio.weight_device fails, as did the C code, with EINVAL. It took me longer than I care to admit to realize the issue: bash's printf uses line-buffering mode on ALL files, and results in two separate write() calls of one line each, while coreutils' printf and our C code was trying to do two lines at once and getting rejected by the kernel. Therefore, we need to split up the code to use cgroups.c for only one device at a time. Also, when thinking about it more, domain_conf.c is the wrong place to worry about major() and minor(); util/cgroup.c already worries about it. So I'm going to propose a v8 of the last two patches, moving a hunk out of 4/5 into 5/5 so that we only use major() and minor() in cgroup-specific code, rather than globally in domain_conf.c. Here's where I got prior to hitting the EINVAL issue with multiple blocks; I'll be posting further edits in the form of v8 soon. diff --git i/src/qemu/qemu_cgroup.c w/src/qemu/qemu_cgroup.c index d397578..99b8105 100644 --- i/src/qemu/qemu_cgroup.c +++ w/src/qemu/qemu_cgroup.c @@ -312,7 +312,8 @@ int qemuSetupCgroup(struct qemud_driver *driver, } } - if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { + if (vm->def->blkio.ndevices && + qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { char *tmp; if (virBlkioDeviceWeightToStr(vm->def->blkio.devices, vm->def->blkio.ndevices, @@ -322,15 +323,13 @@ int qemuSetupCgroup(struct qemud_driver *driver, vm->def->name); goto cleanup; } - if (tmp) { - rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp); - VIR_FREE(tmp); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to set io device weight for domain %s"), - vm->def->name); - goto cleanup; - } + rc = virCgroupSetBlkioDeviceWeight(cgroup, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to set io device weight for domain %s"), + vm->def->name); + goto cleanup; } } diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 01a3fd8..eb5b561 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -5907,7 +5907,8 @@ parseBlkioWeightDeviceStr(char *deviceWeightStr, } } - /* a valid string must have even number of fields */ + /* A valid string must have even number of fields, hence an odd + * number of commas. */ if (!(nsep & 1)) goto error; @@ -6061,7 +6062,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, char *tmp; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid type for device_weight tunable, expected a 'char *'")); + _("invalid type for device_weight tunable, " + "expected a 'char *'")); ret = -1; continue; } @@ -6075,40 +6077,36 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, if (virBlkioDeviceWeightToStr(devices, ndevices, &tmp) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unable to set blkio weight_device tunable")); virBlkioDeviceWeightArrayClear(devices, ndevices); VIR_FREE(devices); ret = -1; continue; } - if (tmp) { - rc = virCgroupSetBlkioDeviceWeight(group, tmp); - VIR_FREE(tmp); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to set blkio weight_device tunable")); - ret = -1; - virBlkioDeviceWeightArrayClear(devices, ndevices); - VIR_FREE(devices); - continue; - } - virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, - vm->def->blkio.ndevices); - VIR_FREE(vm->def->blkio.devices); - vm->def->blkio.devices = devices; - vm->def->blkio.ndevices = ndevices; - } else { + rc = virCgroupSetBlkioDeviceWeight(group, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; virBlkioDeviceWeightArrayClear(devices, ndevices); VIR_FREE(devices); + continue; } + virBlkioDeviceWeightArrayClear(vm->def->blkio.devices, + vm->def->blkio.ndevices); + VIR_FREE(vm->def->blkio.devices); + vm->def->blkio.devices = devices; + vm->def->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); ret = -1; } } - } else if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + } + if (ret < 0) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { /* Clang can't see that if we get here, persistentDef was set. */ sa_assert(persistentDef); @@ -6185,8 +6183,9 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, VIR_TYPED_PARAM_STRING_OKAY, -1); qemuDriverLock(driver); - /* We blindly return a string, and let libvirt.c do the filtering - * on behalf of older clients that can't parse it. */ + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; vm = virDomainFindByUUID(&driver->domains, dom->uuid); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Hu Tao -
Stefan Berger