This makes string can be transported between client and server. For compatibility, o new server should not send strings to old client if it doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY. o new client that wants to be able to send/receive strings should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY; if it is rejected by a old server that doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have a second try with out flag VIR_DOMAIN_TYPED_STRING_OKAY to cope with an old server. Ideas for compatibility are coming from Eric, thanks. --- daemon/remote.c | 32 +++++++++++++++++++++++++++++--- include/libvirt/libvirt.h.in | 25 ++++++++++++++++++++++++- src/libvirt.c | 38 ++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 30 ++++++++++++++++++++++++++++-- src/remote/remote_protocol.x | 2 ++ src/remote_protocol-structs | 2 ++ src/rpc/gendispatch.pl | 2 +- src/util/util.c | 15 +++++++++++++++ src/util/util.h | 2 ++ 9 files changed, 141 insertions(+), 7 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 550bed4..520fef2 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -672,6 +672,15 @@ 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: + if (params[i].value.s) { + val[i].value.remote_typed_param_value_u.s = strdup(params[i].value.s); + if (val[i].value.remote_typed_param_value_u.s == NULL) { + virReportOOMError(); + goto cleanup; + } + } + break; default: virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), params[i].type); @@ -685,8 +694,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; @@ -699,7 +711,7 @@ remoteDeserializeTypedParameters(remote_typed_param *args_params_val, int limit, int *nparams) { - int i; + int i = 0; int rv = -1; virTypedParameterPtr params = NULL; @@ -750,6 +762,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); @@ -760,8 +780,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; } diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c991dfc..fa53147 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -208,6 +208,27 @@ typedef enum { } virDomainModificationImpact; /** + * virDomainFlags: + * + * Flags that can be used with some libvirt APIs. + * + * These enums should not confilict with those of virDomainModificationImpact. + */ +typedef enum { + VIR_DOMAIN_TYPED_STRING_OKAY = 1 << 2, /* Usage of this flag: + * o new server should not send strings to old client if it + * doesn't see the flag VIR_DOMAIN_TYPED_STRING_OKAY. + * o new client that wants to be able to send/receive strings + * should always set the flag VIR_DOMAIN_TYPED_STRING_OKAY; + * if it is rejected by a old server that doesn't understand + * VIR_DOMAIN_TYPED_STRING_OKAY, then the client should have + * a second try without flag VIR_DOMAIN_TYPED_STRING_OKAY to + * cope with an old server. + */ + +} virDomainFlags; + +/** * virDomainInfoPtr: * * a virDomainInfo is a structure filled by virDomainGetInfo() and extracting @@ -489,7 +510,8 @@ 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; /** @@ -520,6 +542,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, see also VIR_DOMAIN_TYPED_STRING_OKAY */ } value; /* parameter value */ }; diff --git a/src/libvirt.c b/src/libvirt.c index f07c720..59d6d26 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3447,6 +3447,23 @@ error: return -1; } +static int virDomainCheckTypedStringFlag(virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + if (!(flags & VIR_DOMAIN_TYPED_STRING_OKAY)) { + int i; + for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + } + } + return 0; +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3472,6 +3489,9 @@ virDomainSetMemoryParameters(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3547,6 +3567,9 @@ virDomainGetMemoryParameters(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3598,6 +3621,9 @@ virDomainSetBlkioParameters(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3657,6 +3683,9 @@ virDomainGetBlkioParameters(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6279,6 +6308,9 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6396,6 +6428,9 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6537,6 +6572,9 @@ int virDomainBlockStatsFlags (virDomainPtr dom, virResetLastError(); + if (virDomainCheckTypedStringFlag(params, *nparams, flags) < 0) + return -1; + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 4dc6974..46738d5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1226,8 +1226,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); } @@ -1276,6 +1279,15 @@ 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: + if (params[i].value.s) { + 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); @@ -1300,7 +1312,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. */ @@ -1347,6 +1359,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); @@ -1357,6 +1377,12 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val, rv = 0; cleanup: + if (rv < 0) { + int j; + for (j = 0; j < i; j++) + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s); + } return rv; } diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f95253e..994c339 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 7894441..9257545 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 b7ac3c8..96340c0 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -439,7 +439,7 @@ elsif ($opt_b) { " $2,\n" . " &n$1)) == NULL)\n" . " goto cleanup;\n"); - push(@free_list, " VIR_FREE(params);"); + push(@free_list, " virTypedParameterFree($1, n$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"; diff --git a/src/util/util.c b/src/util/util.c index 1ff287d..cb9be0a 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2509,3 +2509,18 @@ or other application using the libvirt API.\n\ return 0; } + +void virTypedParameterFree(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); + } + + VIR_FREE(params); +} diff --git a/src/util/util.h b/src/util/util.h index c55e852..8e9c6fb 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -260,4 +260,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 virTypedParameterFree(virTypedParameterPtr params, int nparams); #endif /* __VIR_UTIL_H__ */ -- 1.7.3.1

On 10/25/2011 02:59 PM, Eric Blake wrote: ... Good thing I haven't pushed yet - I just realized another problem. On Set interfaces, it is the client side that must not provide strings without the flag; there, params is valid on function entry, so we can reject if we see mismatch. Hypervisor callbacks must accept the flag, but need not do anything further with it, because the generic libvirt.c wrapper already checked that the flag is only used when valid. But on Get interfaces, like virDomainGetMemoryParamaters, params is an output-only interface; it can be uninitialized memory on entry, so we must not base our behavior on the client's contents. Rather, the typed parameter check has to either be done by each the callback (where we write each hypervisor driver to not output strings unless they first check flags), or can be factored into libvirt.c (hypervisors can blindly write back all parameters, then libvirt.c does a post-process pass that shrinks the array size to skip past all string entries). From a maintainability perspective, making it so hypervisors don't have to repeat the flag check logic is more appealing. So here's what I'd also like to squash in (don't worry, I'll repost the amended series as a v5, for someone else to do a full review, rather than just this piecemeal stuff): diff --git i/src/libvirt.c w/src/libvirt.c index 379bb20..4d2c7c6 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3579,8 +3579,10 @@ error: return -1; } +/* Helper function called to validate incoming client array on any + * interface that sets typed parameters in the hypervisor. */ static int -virTypedParameterStringCheck(virTypedParameterPtr params, +virTypedParameterValidateSet(virTypedParameterPtr params, int nparams, unsigned int flags) { @@ -3597,6 +3599,26 @@ virTypedParameterStringCheck(virTypedParameterPtr params, return 0; } +/* Helper function called to sanitize outgoing hypervisor array on any + * interface that gets typed parameters for the client. */ +static void +virTypedParameterSanitizeGet(virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + if (params && !(flags & VIR_TYPED_PARAM_STRING_OKAY)) { + int i; + for (i = 0; i < *nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) { + VIR_FREE(params[i].value.s); + memmove(&params[i], &params[i + 1], *nparams - i + 1); + --i; + --*nparams; + } + } + } +} + /** * virDomainSetMemoryParameters: * @domain: pointer to domain object @@ -3622,7 +3644,7 @@ virDomainSetMemoryParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3700,9 +3722,6 @@ virDomainGetMemoryParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3720,6 +3739,7 @@ virDomainGetMemoryParameters(virDomainPtr domain, ret = conn->driver->domainGetMemoryParameters (domain, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -3754,7 +3774,7 @@ virDomainSetBlkioParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -3816,9 +3836,6 @@ virDomainGetBlkioParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -3836,6 +3853,7 @@ virDomainGetBlkioParameters(virDomainPtr domain, ret = conn->driver->domainGetBlkioParameters (domain, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError (VIR_ERR_NO_SUPPORT, __FUNCTION__); @@ -6386,9 +6404,6 @@ virDomainGetSchedulerParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, *nparams, 0) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6407,6 +6422,7 @@ virDomainGetSchedulerParameters(virDomainPtr domain, ret = conn->driver->domainGetSchedulerParameters (domain, params, nparams); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, 0); return ret; } @@ -6447,9 +6463,6 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN(domain)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6469,6 +6482,7 @@ virDomainGetSchedulerParametersFlags(virDomainPtr domain, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } @@ -6504,7 +6518,7 @@ virDomainSetSchedulerParameters(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, 0) < 0) + if (virTypedParameterValidateSet(params, nparams, 0) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -6570,7 +6584,7 @@ virDomainSetSchedulerParametersFlags(virDomainPtr domain, virResetLastError(); - if (virTypedParameterStringCheck(params, nparams, flags) < 0) + if (virTypedParameterValidateSet(params, nparams, flags) < 0) return -1; if (!VIR_IS_CONNECTED_DOMAIN(domain)) { @@ -6714,9 +6728,6 @@ int virDomainBlockStatsFlags (virDomainPtr dom, virResetLastError(); - if (virTypedParameterStringCheck(params, *nparams, flags) < 0) - return -1; - if (!VIR_IS_CONNECTED_DOMAIN (dom)) { virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); virDispatchError(NULL); @@ -6733,6 +6744,7 @@ int virDomainBlockStatsFlags (virDomainPtr dom, ret = conn->driver->domainBlockStatsFlags(dom, path, params, nparams, flags); if (ret < 0) goto error; + virTypedParameterSanitizeGet(params, nparams, flags); return ret; } virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

This patch adds a parameter --weight-device to virsh command blkiotune for setting/getting blkio.weight_device. --- daemon/remote.c | 2 +- include/libvirt/libvirt.h.in | 9 ++ src/conf/domain_conf.c | 129 ++++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++ src/libvirt_private.syms | 2 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 199 +++++++++++++++++++++++++++++++++++++++-- src/util/cgroup.c | 33 +++++++ src/util/cgroup.h | 3 + tools/virsh.c | 69 +++++++++++++-- tools/virsh.pod | 5 +- 11 files changed, 467 insertions(+), 22 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 520fef2..7691b08 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1619,7 +1619,7 @@ success: cleanup: if (rv < 0) virNetMessageSaveError(rerr); - VIR_FREE(params); + virTypedParameterFree(params, nparams); if (dom) virDomainFree(dom); return rv; diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index fa53147..84178c2 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1162,6 +1162,15 @@ char * virDomainGetSchedulerType(virDomainPtr domain, #define VIR_DOMAIN_BLKIO_WEIGHT "weight" +/** + * VIR_DOMAIN_BLKIO_WEIGHT_DEVICE: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight. This name refers to a VIR_TYPED_PARAM_STRING. + */ + +#define VIR_DOMAIN_BLKIO_WEIGHT_DEVICE "weight_device" + /* 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 b9ddf26..96723ec 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -580,6 +580,97 @@ VIR_ENUM_IMPL(virDomainNumatuneMemMode, VIR_DOMAIN_NUMATUNE_MEM_LAST, #define VIR_DOMAIN_XML_WRITE_FLAGS VIR_DOMAIN_XML_SECURE #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE + +void virBlkioWeightDeviceFree(virBlkioWeightDevicePtr deviceWeights, + int ndevices) +{ + int i; + + for (i = 0; i < ndevices; i++) + VIR_FREE(deviceWeights[i].path); + VIR_FREE(deviceWeights); +} + +/** + * virBlkioWeightDeviceToStr: + * + * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of weight devices. + */ +#if defined(major) && defined(minor) +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i < ndevices; i++) { + if (stat(weightdevices[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), + weightdevices[i].weight); + } + + *result = virBufferContentAndReset(&buf); + return 0; +} +#else +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1; +} +#endif + +/** + * virDomainBlkioWeightDeviceParseXML + * + * this function parses a XML node: + * + * <device> + * <path>/fully/qualified/device/path</path> + * <weight>weight</weight> + * </device> + * + * and fills a virBlkioWeightDevice struct. + */ +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root, + virBlkioWeightDevicePtr 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) { @@ -1244,6 +1335,8 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->emulator); VIR_FREE(def->description); + virBlkioWeightDeviceFree(def->blkio.weight_devices, def->blkio.ndevices); + virDomainWatchdogDefFree(def->watchdog); virDomainMemballoonDefFree(def->memballoon); @@ -6520,6 +6613,20 @@ 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.weight_devices, n) < 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i < n; i++) { + virDomainBlkioWeightDeviceParseXML(nodes[i], &def->blkio.weight_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) @@ -10572,10 +10679,26 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->mem.cur_balloon); /* add blkiotune only if there are any */ - if (def->blkio.weight) { + if (def->blkio.weight || def->blkio.weight_devices) { virBufferAsprintf(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); + + if (def->blkio.weight_devices) { + int i; + + for (i = 0; i < def->blkio.ndevices; i++) { + virBufferAsprintf(buf, " <device>\n"); + virBufferAsprintf(buf, " <path>%s</path>\n", + def->blkio.weight_devices[i].path); + virBufferAsprintf(buf, " <weight>%d</weight>\n", + def->blkio.weight_devices[i].weight); + virBufferAsprintf(buf, " </device>\n"); + } + } + virBufferAsprintf(buf, " </blkiotune>\n"); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8765f32..140369c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1297,6 +1297,20 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ }; +typedef struct _virBlkioWeightDevice virBlkioWeightDevice; +typedef virBlkioWeightDevice *virBlkioWeightDevicePtr; +struct _virBlkioWeightDevice { + char *path; + int weight; +}; + +void virBlkioWeightDeviceFree(virBlkioWeightDevicePtr deviceWeights, + int ndevices); +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1314,6 +1328,8 @@ struct _virDomainDef { struct { unsigned int weight; + int ndevices; + virBlkioWeightDevicePtr weight_devices; } blkio; struct { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d79f65c..00099c0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,8 @@ virDomainAuditVcpu; # domain_conf.h +virBlkioWeightDeviceFree; +virBlkioWeightDeviceToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..ff13965 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 (virBlkioWeightDeviceToStr(vm->def->blkio.weight_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 = virCgroupSetBlkioWeightDevice(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 4ea3236..179ecb4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -89,6 +89,7 @@ #include "locking/lock_manager.h" #include "locking/domain_lock.h" #include "virkeycode.h" +#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -112,7 +113,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); @@ -5847,6 +5848,73 @@ cleanup: return ret; } +/* weightDeviceStr 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(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size) +{ + char *temp; + int nDevice = 0; + int i; + virBlkioWeightDevicePtr result = NULL; + + temp = (char *)weightDeviceStr; + while (temp) { + temp = strchr(temp, ';'); + if (temp) { + temp++; + if (*temp == '\0') + break; + nDevice++; + } + } + nDevice++; + + if (VIR_ALLOC_N(result, nDevice) < 0) { + virReportOOMError(); + return -1; + } + + i = 0; + temp = (char *)weightDeviceStr; + while (temp && i < nDevice) { + char *p = temp; + + /* device path */ + + p = strchr(p, ','); + if (!p) + goto fail; + + result[i].path = strndup(temp, p - temp); + + /* weight */ + + temp = p + 1; + if (!temp) + goto fail; + + if (virStrToLong_i(temp, &p, 10, &result[i].weight) < 0) + goto fail; + + i++; + if (*p == '\0') + break; + else if (*p != ';') + goto fail; + temp = p + 1; + } + + *dw = result; + *size = i; + + return 0; + +fail: + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5859,9 +5927,11 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; bool isActive; + bool typed_string; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_TYPED_STRING_OKAY, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5874,6 +5944,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, isActive = virDomainObjIsActive(vm); + typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; @@ -5913,10 +5985,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { + int rc; virTypedParameterPtr param = &params[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'")); @@ -5937,6 +6009,46 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, _("unable to set blkio weight tunable")); ret = -1; } + } else if(STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) { + int ndevices; + virBlkioWeightDevicePtr weightDevices = NULL; + char *tmp; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for blkio weight_device tunable, expected a 'char *'")); + ret = -1; + continue; + } + + if (parseBlkioWeightDeviceStr(params[i].value.s, + &weightDevices, + &ndevices) < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid format for blkio weight_device")); + ret = -1; + continue; + } + if (virBlkioWeightDeviceToStr(weightDevices, + ndevices, + &tmp) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to set blkio weight_device tunable")); + virBlkioWeightDeviceFree(weightDevices, ndevices); + ret = -1; + continue; + } + virBlkioWeightDeviceFree(weightDevices, ndevices); + VIR_FREE(weightDevices); + if (tmp) { + rc = virCgroupSetBlkioWeightDevice(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); @@ -5966,6 +6078,21 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } persistentDef->blkio.weight = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE)) { + virBlkioWeightDevicePtr tmp = NULL; + int ndevices; + if (parseBlkioWeightDeviceStr(params[i].value.s, + &tmp, + &ndevices) < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid device weight format: %s"), + params[i].value.s); + ret = -1; + continue; + } + virBlkioWeightDeviceFree(persistentDef->blkio.weight_devices, persistentDef->blkio.ndevices); + persistentDef->blkio.weight_devices = tmp; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5991,7 +6118,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; @@ -5999,9 +6126,11 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, int ret = -1; int rc; bool isActive; + bool typed_string; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); + VIR_DOMAIN_AFFECT_CONFIG | + VIR_DOMAIN_TYPED_STRING_OKAY, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6014,19 +6143,32 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if ((*nparams) == 0) { /* Current number of blkio parameters supported by cgroups */ - *nparams = QEMU_NB_BLKIO_PARAM; + if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) + *nparams = QEMU_NB_BLKIO_PARAM; + else + *nparams = QEMU_NB_BLKIO_PARAM - 1; ret = 0; goto cleanup; } - if ((*nparams) != QEMU_NB_BLKIO_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; + if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) { + if ((*nparams) != QEMU_NB_BLKIO_PARAM) { + qemuReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto cleanup; + } + } else { + if ((*nparams) != QEMU_NB_BLKIO_PARAM - 1) { + qemuReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto cleanup; + } } isActive = virDomainObjIsActive(vm); + typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; @@ -6065,6 +6207,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < *nparams; i++) { + char *weight_device; virTypedParameterPtr param = &params[i]; val = 0; param->value.ui = 0; @@ -6085,6 +6228,23 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } param->value.ui = val; break; + case 1: /* blkio.weight_device */ + if (typed_string) { + param->type = VIR_TYPED_PARAM_STRING; + rc = virCgroupGetBlkioWeightDevice(group, &weight_device); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get blkio weight_device")); + goto cleanup; + } + if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field blkio weight_device too long for destination")); + goto cleanup; + } + param->value.s = weight_device; + } + break; default: break; @@ -6108,6 +6268,25 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break; + case 1: /* blkio.weight_device */ + if (typed_string && persistentDef->blkio.ndevices > 0) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (j = 0; j < persistentDef->blkio.ndevices; j++) { + virBufferAsprintf(&buf, "%s %d\n", + persistentDef->blkio.weight_devices[j].path, + persistentDef->blkio.weight_devices[j].weight); + } + + param->value.s = virBufferContentAndReset(&buf); + param->type = VIR_TYPED_PARAM_STRING; + } + if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field blkio weight_device too long for destination")); + goto cleanup; + } + break; + default: break; /* should not hit here */ diff --git a/src/util/cgroup.c b/src/util/cgroup.c index c8d1f33..f6e5a08 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -982,6 +982,39 @@ int virCgroupGetBlkioWeight(virCgroupPtr group, unsigned int *weight) } /** + * virCgroupSetBlkioWeightDevice: + * + * @group: The cgroup to change io weight device for + * @weight_device: The Weight device for this cgroup + * + * Returns: 0 on success + */ +int virCgroupSetBlkioWeightDevice(virCgroupPtr group, + const char *weight_device) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", + weight_device); +} + +/** + * virCgroupGetBlkioWeightDevice: + * + * @group: The cgroup to get weight_device for + * @weight_device: returned weight_device string + * + * Returns: 0 on success + */ +int virCgroupGetBlkioWeightDevice(virCgroupPtr group, + char **weight_device) +{ + return virCgroupGetValueStr(group, + VIR_CGROUP_CONTROLLER_BLKIO, + "blkio.weight_device", weight_device); +} + +/** * virCgroupSetMemory: * * @group: The cgroup to change memory for diff --git a/src/util/cgroup.h b/src/util/cgroup.h index d190bb3..87e196b 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 virCgroupSetBlkioWeightDevice(virCgroupPtr group, const char *weight_device); +int virCgroupGetBlkioWeightDevice(virCgroupPtr group, char **weight_device); + int virCgroupSetMemory(virCgroupPtr group, unsigned long long kb); int virCgroupGetMemoryUsage(virCgroupPtr group, unsigned long *kb); diff --git a/tools/virsh.c b/tools/virsh.c index 067d3e5..c85f5b6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4644,6 +4644,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]")}, + {"weight-device", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weight, 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")}, @@ -4654,6 +4656,7 @@ static bool cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) { virDomainPtr dom; + const char *weight_device = NULL; int weight = 0; int nparams = 0; int rv = 0; @@ -4698,12 +4701,36 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } } + rv = vshCommandOptString(cmd, "weight-device", &weight_device); + 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 */ + /* old libvirtd doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, we + * give it a second try with this flag disabled in the case of an + * old libvirtd. */ + flags |= VIR_DOMAIN_TYPED_STRING_OKAY; if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { - vshError(ctl, "%s", - _("Unable to get number of blkio parameters")); - goto cleanup; + if (last_error->code == VIR_ERR_INVALID_ARG) { + flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + nparams = 0; + if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { + vshError(ctl, "%s", + _("Unable to get number of blkio parameters")); + goto cleanup; + } + } else { + vshError(ctl, "%s", + _("Unable to get number of blkio parameters")); + goto cleanup; + } } if (nparams == 0) { @@ -4745,6 +4772,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,11 +4796,35 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); weight = 0; } + + if (weight_device) { + temp->value.s = (char *)weight_device; + temp->type = VIR_TYPED_PARAM_STRING; + strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE, + sizeof(temp->field)); + } + } + ret = true; + if (weight_device) { + flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + if (last_error->code == VIR_ERR_INVALID_ARG) { + flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + vshError(ctl, "%s", _("Unable to change blkio parameters")); + ret = false; + } + } else { + vshError(ctl, "%s", _("Unable to change blkio parameters")); + ret = false; + } + } + } else { + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + vshError(ctl, "%s", _("Unable to change blkio parameters")); + ret = false; + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) - vshError(ctl, "%s", _("Unable to change blkio parameters")); - else - ret = true; } cleanup: diff --git a/tools/virsh.pod b/tools/virsh.pod index 7712db4..8c29759 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1030,12 +1030,15 @@ 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<--weight-device> B<weight-device>] [[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<weight-device> is in the format of /device/path,weight;/device/path,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 10/12/2011 03:26 AM, Hu Tao wrote: I had to tweak this one to apply on top of my proposed changes to the other one; I'll repost the total changes as a v5. This should be squashed into 1/2. We should document what format that string takes. Yes, virsh should also document it, but not all clients go through virsh, so just copy the virsh listing here: /device/path,weight;/device/path,weight. Which means we should probably _also_ do some input validation, and reject /device/path names that contain ','. I don't like this pattern of creating vir*Free functions with multiple arguments. Either we should be freeing just one struct (single path/weight pair, caller iterates over the array to free it one struct at a time), or encapsulating the entire array of path/weight pairs, and the number of those pairs, into a single struct (caller frees a single struct with one parameter). And yes, I know we did it with virTypedParameterFree, at my suggestion. This is making me think all the more that we should update the name of that function, and push the free back to the caller. This should check for buffer error. The caller can't issue a good error message - it doesn't know if the failure was due to stat() failure or to OOM. You only have two callers, which both reported VIR_ERR_INTERNAL_ERROR, but I'm wondering if that should be improved - if the error is user-visible, we should try harder to give a more accurate message. But I didn't change that. This won't validate without edits to docs/schemas/domaincommon.rng. Also, we have to document the new XML in docs/formatdomain.html.in. We should split this patch into (at least) two parts - the public API and clients that react to that API (docs, include, virsh), and the qemu-specific implementation of the new public interface. Here, I'm thinking its better to have a for loop, freeing each weight_devices[i]. The for loop on ndevices renders this NULL check redundant. virBufferAddLit is faster, when it works. Must be escaped, in case it contains characters that would interfere with xml parsing. size_t for array length Typically, if we use 'name' for an array, then we use 'nname' for its length; meaning this should either be ndevices/devices, or nweight_devices/weight_devices. I prefer the short name 'device', not to mention that 'device_weight' reads better than 'weight_device' if we were to go with a longer name. 'make syntax-check' rejected this, since you didn't use any of the functions in that header. Make temp const char*, or remove const from weightDeviceStr, instead of casting away const. Memory leak - if you parse the first path, but the second strndup fails, then you leak result[0].path. Unused variable. I've run out of time to finish this review today, but so far, I'm squashing in at least this: diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index 215a46f..4a5aefc 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -629,6 +629,9 @@ int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices, weightdevices[i].weight); } + if (virBufferError(&buf)) + return -1; + *result = virBufferContentAndReset(&buf); return 0; } @@ -6677,7 +6680,8 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, } for (i = 0; i < n; i++) { - virDomainBlkioWeightDeviceParseXML(nodes[i], &def->blkio.weight_devices[i]); + virDomainBlkioWeightDeviceParseXML(nodes[i], + &def->blkio.weight_devices[i]); } def->blkio.ndevices = n; VIR_FREE(nodes); @@ -10763,17 +10767,13 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " <weight>%u</weight>\n", def->blkio.weight); - if (def->blkio.weight_devices) { - int i; - - for (i = 0; i < def->blkio.ndevices; i++) { - virBufferAsprintf(buf, " <device>\n"); - virBufferAsprintf(buf, " <path>%s</path>\n", - def->blkio.weight_devices[i].path); - virBufferAsprintf(buf, " <weight>%d</weight>\n", - def->blkio.weight_devices[i].weight); - virBufferAsprintf(buf, " </device>\n"); - } + for (n = 0; n < def->blkio.ndevices; n++) { + virBufferAddLit(buf, " <device>\n"); + virBufferEscapeString(buf, " <path>%s</path>\n", + def->blkio.weight_devices[n].path); + virBufferAsprintf(buf, " <weight>%d</weight>\n", + def->blkio.weight_devices[n].weight); + virBufferAddLit(buf, " </device>\n"); } virBufferAsprintf(buf, " </blkiotune>\n"); diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 7060111..caf0615 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -89,7 +89,6 @@ #include "locking/lock_manager.h" #include "locking/domain_lock.h" #include "virkeycode.h" -#include "c-ctype.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -5895,14 +5894,16 @@ cleanup: /* weightDeviceStr 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(const char *weightDeviceStr, virBlkioWeightDevicePtr *dw, int *size) +static int +parseBlkioWeightDeviceStr(char *weightDeviceStr, + virBlkioWeightDevicePtr *dw, int *size) { char *temp; int nDevice = 0; int i; virBlkioWeightDevicePtr result = NULL; - temp = (char *)weightDeviceStr; + temp = weightDeviceStr; while (temp) { temp = strchr(temp, ';'); if (temp) { @@ -5920,7 +5921,7 @@ static int parseBlkioWeightDeviceStr(const char *weightDeviceStr, virBlkioWeight } i = 0; - temp = (char *)weightDeviceStr; + temp = weightDeviceStr; while (temp && i < nDevice) { char *p = temp; @@ -5988,8 +5989,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, isActive = virDomainObjIsActive(vm); - typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + typed_string = (flags & VIR_TYPED_PARAM_STRING_OKAY) != 0; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; @@ -6187,7 +6188,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if ((*nparams) == 0) { /* Current number of blkio parameters supported by cgroups */ - if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) + if (flags & VIR_TYPED_PARAM_STRING_OKAY) *nparams = QEMU_NB_BLKIO_PARAM; else *nparams = QEMU_NB_BLKIO_PARAM - 1; @@ -6195,7 +6196,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - if (flags & VIR_DOMAIN_TYPED_STRING_OKAY) { + if (flags & VIR_TYPED_PARAM_STRING_OKAY) { if ((*nparams) != QEMU_NB_BLKIO_PARAM) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid parameter count")); @@ -6211,8 +6212,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, isActive = virDomainObjIsActive(vm); - typed_string = (flags & VIR_DOMAIN_TYPED_STRING_OKAY) == VIR_DOMAIN_TYPED_STRING_OKAY; - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + typed_string = (flags & VIR_TYPED_PARAM_STRING_OKAY) != 0; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; diff --git i/tools/virsh.c w/tools/virsh.c index 58e7894..7197105 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -4717,13 +4717,13 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (nparams == 0) { /* get the number of blkio parameters */ - /* old libvirtd doesn't understand VIR_DOMAIN_TYPED_STRING_OKAY, we + /* old libvirtd doesn't understand VIR_TYPED_PARAM_STRING_OKAY, we * give it a second try with this flag disabled in the case of an * old libvirtd. */ - flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { if (last_error->code == VIR_ERR_INVALID_ARG) { - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; nparams = 0; if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { vshError(ctl, "%s", @@ -4810,10 +4810,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) } ret = true; if (weight_device) { - flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + flags |= VIR_TYPED_PARAM_STRING_OKAY; if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { if (last_error->code == VIR_ERR_INVALID_ARG) { - flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); ret = false; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org