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> --- v6: address review comments from Stefan, defer filtering of string params on Get functions to the rpc side 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 7181f62..e10a72b 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 bd52b21..b25f8db 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2572,3 +2572,17 @@ or other application using the libvirt API.\n\ return 0; } + +void +virTypedParameterArrayClear(virTypedParameterPtr params, int nparams) +{ + int i; + + if (!params) + return; + + for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s); + } +} diff --git a/src/util/util.h b/src/util/util.h index e869f1b..e5594cb 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -263,4 +263,6 @@ bool virIsDevMapperDevice(const char *dev_name) ATTRIBUTE_NONNULL(1); int virEmitXMLWarning(int fd, const char *name, const char *cmd) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + +void virTypedParameterArrayClear(virTypedParameterPtr params, int nparams); #endif /* __VIR_UTIL_H__ */ -- 1.7.4.4

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> --- v6: address review comments from Stefan, add filtering of string params on Get functions 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 ea7fb24..9d689af 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.4.4

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. --- v6: no real change 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.4.4

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". --- v5: did not include this patch v6: split v4 2/2 into two parts; this part covers just the libvirt.h and virsh changes. Rename public-facing terminology to be "device_weight", not "weight_device", since we are talking about per-device weights and may later add other per-device attributes (no need to propagate cgroups' stupid naming to the user). Rebase to fit in with VIR_TYPED_PARAM_STRING tweaks earlier in series. Should we use just commas for field separation, rather than commas between device and weight but semicolon between device-weight pairs? It would make it much easier to do: virsh blkiotune dom --device-weight /dev/sda,500,/dev/sdb,500 instead of requiring shell quoting: virsh blkiotune dom --device-weight '/dev/sda,500;/dev/sdb,500' docs/formatdomain.html.in | 29 ++++++++- docs/schemas/domaincommon.rng | 12 ++++ include/libvirt/libvirt.h.in | 10 +++ src/conf/domain_conf.c | 130 ++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 17 +++++ src/libvirt_private.syms | 2 + tools/virsh.c | 32 +++++++++- tools/virsh.pod | 6 ++- 8 files changed, 228 insertions(+), 10 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..e848b7d 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.7</span></dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..6f96fe2 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"> + <data type="int"/> + </element> + </interleave> + </element> + </zeroOrMore> </element> </optional> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e10a72b..5ae7d10 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 238edfd..e5b5f69 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -603,6 +603,99 @@ 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)) + 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_i(c, NULL, 10, &dw->weight) < 0) + return -1; + VIR_FREE(c); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1272,6 +1365,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); @@ -6710,6 +6807,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; + } + + for (i = 0; i < n; i++) { + virDomainBlkioDeviceWeightParseXML(nodes[i], + &def->blkio.devices[i]); + } + def->blkio.ndevices = n; + VIR_FREE(nodes); + } + /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, &def->mem.hard_limit) < 0) @@ -10848,10 +10960,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>%d</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..577d10d 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; + 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..ca8db70 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"); } @@ -4769,11 +4786,20 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight; + temp->type = VIR_TYPED_PARAM_STRING; + strncpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field)); + } } - 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.4.4

From: Hu Tao <hutao@cn.fujitsu.com> Implement setting/getting per-device blkio weights in qemu, using the cgroups blkio.weight_device tunable. --- v5: did not include this patch v6: split v4 2/2 into two parts; this part covers just the qemu. Rebase to accomodate 'device_weight' naming and VIR_TYPED_PARAM_STRING handling. This patch is broken, with several flaws: 1. 'virsh blkiotune dom --device-weight /dev/sda,500 --live' does not alter the 'virsh dumpxml dom' live XML. The cgroups change is active, but unless we also reflect the change in the eventual xml dump, then we aren't accurately telling the user the current state of the domain. This means that qemuDomainSetBlkioParameters needs a fix. 2. 'virsh blkiotune dom --live' now shows output that looks like: weight : 500 device_weight : 8,0 500 whereas 'virsh blkiotune dom --config' shows: weight : 500 device_weight : /dev/sda,500 The --config version is correct - it accurately reflects the XML. The --live version is wrong - we MUST NOT expose major,minor back to the user, since we intentionally decided that the XML should track only the device name, not the major/minor implementation detail of cgroups. I think the correct fix would be to just delete virCgroupGetBlkioDeviceWeight and treat cgroups blkio.weight_device as write-only, and instead have qemuDomainGetBlkioParameters always produce output from the domain definition (--live vs. --config merely choosing between vm->def vs. persistentDef), which stems back to my solution of point 1 in having in vm->def accurately reflect whatever is written into cgroups. src/qemu/qemu_cgroup.c | 22 ++++++ src/qemu/qemu_driver.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 33 ++++++++ src/util/cgroup.h | 4 + 4 files changed, 245 insertions(+), 5 deletions(-) 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..6b0acf0 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,82 @@ 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 i; + virBlkioDeviceWeightPtr result = NULL; + + temp = deviceWeightStr; + while (temp) { + temp = strchr(temp, ';'); + if (temp) { + temp++; + if (*temp == '\0') + break; + ndevices++; + } + } + ndevices++; + + 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_i(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 +6030,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 +6054,45 @@ 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; + } + virBlkioDeviceWeightArrayClear(devices, ndevices); + VIR_FREE(devices); + if (tmp) { + rc = virCgroupSetBlkioDeviceWeight(group, tmp); + VIR_FREE(tmp); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; + continue; + } + } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6007,9 +6122,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 +6162,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 +6176,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); @@ -6104,6 +6235,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < *nparams && i < QEMU_NB_BLKIO_PARAM; i++) { + char *device_weights; virTypedParameterPtr param = ¶ms[i]; val = 0; param->value.ui = 0; @@ -6125,6 +6257,23 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkiotune.device_weight */ + param->type = VIR_TYPED_PARAM_STRING; + rc = virCgroupGetBlkioDeviceWeight(group, &device_weights); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get blkio device weights")); + goto cleanup; + } + 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; + } + param->value.s = device_weights; + break; default: break; @@ -6149,6 +6298,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,%d", + 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..458bf91 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -982,6 +982,39 @@ 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 + * + * Returns: 0 on success + */ +int virCgroupSetBlkioDeviceWeight(virCgroupPtr group, + const char *device_weight) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + device_weight); +} + +/** + * virCgroupGetBlkioDeviceWeight: + * + * @group: The cgroup to get weight_device for + * @device_weight: returned device_weight string + * + * Returns: 0 on success + */ +int virCgroupGetBlkioDeviceWeight(virCgroupPtr group, + char **device_weight) +{ + return virCgroupGetValueStr(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..2eb7307 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -55,6 +55,10 @@ 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 virCgroupGetBlkioDeviceWeight(virCgroupPtr group, char **device_weight); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); -- 1.7.4.4