[libvirt] [PATCH v3 0/2] add blkio.weight_device support

This series adds support for blkio.weight_device. changes from v2: - fix the compatibility issue of TYPED_STRING with old clients and old servers - modify XML format for weight_device as danp suggested Hu Tao (2): Add VIR_TYPED_PARAM_STRING add interface for blkio.weight_device daemon/remote.c | 22 +++++ include/libvirt/libvirt.h.in | 14 +++- src/conf/domain_conf.c | 114 ++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 191 +++++++++++++++++++++++++++++++++++++++++- src/remote/remote_driver.c | 17 ++++ src/remote/remote_protocol.x | 2 + src/remote_protocol-structs | 2 + src/util/cgroup.c | 33 +++++++ src/util/cgroup.h | 3 + tools/virsh.c | 52 ++++++++++-- tools/virsh.pod | 5 +- 14 files changed, 479 insertions(+), 15 deletions(-) -- 1.7.3.1

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 of compatibility are coming from Eric, thanks. --- daemon/remote.c | 17 +++++++++++++++++ include/libvirt/libvirt.h.in | 5 ++++- src/remote/remote_driver.c | 17 +++++++++++++++++ src/remote/remote_protocol.x | 2 ++ src/remote_protocol-structs | 2 ++ 5 files changed, 42 insertions(+), 1 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 245d41c..82ee13b 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); @@ -750,6 +759,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); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 39155a6..448a0e7 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -205,6 +205,7 @@ 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. */ + VIR_DOMAIN_TYPED_STRING_OKAY = 1 << 2, } virDomainModificationImpact; /** @@ -489,7 +490,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 +522,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 */ } value; /* parameter value */ }; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1217d94..85eaeea 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1276,6 +1276,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); @@ -1347,6 +1356,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); diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 455e324..ec2fc84 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 810b19c..9ddd4e1 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 { -- 1.7.3.1

On 09/23/2011 12:40 AM, Hu Tao wrote:
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.
These points should probably be documented in libvirt.h.in, as well. I'm also thinking that libvirt.c should do some argument validation - for every API that uses a virTypedParameter array, it should validate that no VIR_TYPED_PARAM_STRING occurs if the flag is not present.
+++ 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:
Memory leak. The cleanup: label must free any successfully allocated remote_typed_param_value_u.s contents prior to the failure point.
virNetError(VIR_ERR_RPC, _("unknown parameter type: %d"), params[i].type); @@ -750,6 +759,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);
Memory leak. The cleanup: label must now iterate over params, and free any successfully allocated params[i].value.s allocated prior to a failure.
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 39155a6..448a0e7 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -205,6 +205,7 @@ 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. */ + VIR_DOMAIN_TYPED_STRING_OKAY = 1<< 2, } virDomainModificationImpact;
I'm not sure if this is the best place to stick this enum value; it might fit better if it is listed (and documented!) closer to the rest of virTypedParameterType stuff. But I agree with setting it to the value of 1<<2, since all current clients of virTypedParameterType seem to be using VIR_DOMAIN_AFFECT_* and nothing further.
/** @@ -489,7 +490,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 */
Put a trailing comma after 7, so that the next addition (if any) doesn't have to touch two lines like this addition did.
} virTypedParameterType;
/** @@ -520,6 +522,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 */
Here, I'd use: char *s; /* type is STRING, see also VIR_DOMAIN_TYPED_STRING_OKAY */
} value; /* parameter value */ };
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1217d94..85eaeea 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1276,6 +1276,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;
Counterpart memory leak.
default: remoteError(VIR_ERR_RPC, _("unknown parameter type: %d"), params[i].type); @@ -1347,6 +1356,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;
And another memory leak. -- Eric Blake eblake@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 | 5 + include/libvirt/libvirt.h.in | 9 ++ src/conf/domain_conf.c | 114 ++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 191 +++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 33 +++++++ src/util/cgroup.h | 3 + tools/virsh.c | 52 ++++++++++-- tools/virsh.pod | 5 +- 11 files changed, 437 insertions(+), 14 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 82ee13b..bee1b94 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1566,6 +1566,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, int nparams = args->nparams; unsigned int flags; int rv = -1; + int i; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -1610,6 +1611,10 @@ success: cleanup: if (rv < 0) virNetMessageSaveError(rerr); + for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s); + } VIR_FREE(params); if (dom) virDomainFree(dom); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 448a0e7..a1f2c98 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1139,6 +1139,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. + */ + +#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 7463d7c..75b17cd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -566,6 +566,82 @@ 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 +/** + * 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. + */ +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr weightdevices, + int ndevices, + char **result) +{ + int i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i < ndevices; i++) { + virBufferAsprintf(&buf, "%d:%d %d\n", + weightdevices[i].major, + weightdevices[i].minor, + weightdevices[i].weight); + } + + *result = virBufferContentAndReset(&buf); + return 0; +} + +/** + * virDomainBlkioWeightDeviceParseXML + * + * this function parses a XML node: + * + * <device> + * <path>/fully/qulaified/device/path</path> + * <weight>weight</weight> + * </device> + * + * and fills a virBlkioWeightDevice struct. + */ +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root, + virBlkioWeightDevicePtr dw) +{ + char *c; + struct stat s; + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + c = (char *)xmlNodeGetContent(node); + if (!c) + return -1; + if (stat(c, &s) == -1) + return -1; + if ((s.st_mode & S_IFMT) == S_IFBLK) { + dw->major = major(s.st_rdev); + dw->minor = minor(s.st_rdev); + } else + return -1; + dw->path = (char *)xmlNodeGetContent(node); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + dw->weight = atoi(c); + VIR_FREE(c); + } + } + node = node->next; + } + + return 0; +} + + + static void virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED) { @@ -1230,6 +1306,8 @@ void virDomainDefFree(virDomainDefPtr def) VIR_FREE(def->emulator); VIR_FREE(def->description); + VIR_FREE(def->blkio.weight_devices); + virDomainWatchdogDefFree(def->watchdog); virDomainMemballoonDefFree(def->memballoon); @@ -6464,6 +6542,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) @@ -10499,10 +10591,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 371f270..562c530 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1276,6 +1276,20 @@ struct _virDomainNumatuneDef { /* Future NUMA tuning related stuff should go here. */ }; +typedef struct _virBlkioWeightDevice virBlkioWeightDevice; +typedef virBlkioWeightDevice *virBlkioWeightDevicePtr; +struct _virBlkioWeightDevice { + int major; + int minor; + char *path; + int weight; +}; + +int virBlkioWeightDeviceToStr(virBlkioWeightDevicePtr deviceWeights, + int ndevices, + char **result); + + /* * Guest VM main configuration * @@ -1293,6 +1307,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 8235ea1..34ec488 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -227,6 +227,7 @@ virDomainAuditVcpu; # domain_conf.h +virBlkioWeightDeviceToStr; virDiskNameToBusDeviceIndex; virDiskNameToIndex; virDomainActualNetDefFree; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 2a10bd2..eb06871 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)) { + for (i = 0; i < vm->def->blkio.ndevices; i++) { + char *tmp = NULL; + + virAsprintf(&tmp, "%d:%d %d", + vm->def->blkio.weight_devices[i].major, + vm->def->blkio.weight_devices[i].minor, + vm->def->blkio.weight_devices[i].weight); + if (tmp) { + rc = virCgroupSetBlkioWeightDevice(cgroup, tmp); + VIR_FREE(tmp); + tmp = NULL; + 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 0d0bea2..0d17ccd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -44,6 +44,7 @@ #include <sys/ioctl.h> #include <sys/un.h> #include <byteswap.h> +#include <ctype.h> #include "qemu_driver.h" @@ -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); @@ -5794,6 +5795,96 @@ 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) +{ + struct stat s; + const char *temp; + int nDevice = 0; + int i, len; + virBlkioWeightDevicePtr result = NULL; + + if (!dw) + return -1; + if (*dw) + return -1; + + temp = 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 = weightDeviceStr; + while (temp && i < nDevice) { + const char *p = temp; + + /* device path */ + + while (*p != ',' && *p != '\0') + ++p; + if (*p == '\0') + goto fail; + len = p - temp + 1; + if (VIR_ALLOC_N(result[i].path, len) < 0) + goto fail; + memcpy(result[i].path, temp, len - 1); + result[i].path[len - 1] = '\0'; + + if (stat(result[i].path, &s) == -1) { + VIR_FREE(result[i].path); + goto fail; + } + if ((s.st_mode & S_IFMT) != S_IFBLK) { + VIR_FREE(result[i].path); + goto fail; + } + result[i].major = major(s.st_rdev); + result[i].minor = minor(s.st_rdev); + + /* weight */ + + temp = p + 1; + if (!temp) + goto fail; + + p = temp; + while (isdigit(*p) && ++p); + if (!p || (*p != ';' && *p != '\0')) + goto fail; + + result[i].weight = atoi(temp); + + i++; + if (*p == '\0') + break; + temp = p + 1; + } + + *dw = result; + *size = i; + + return 0; + +fail: + VIR_FREE(result); + return -1; +} + static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5860,10 +5951,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'")); @@ -5884,6 +5975,44 @@ 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 tmp = NULL; + 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, + &tmp, + &ndevices) < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid format for blkio weight_device")); + ret = -1; + continue; + } + for (i = 0; i < ndevices; i++) { + char *weight_device = NULL; + + virAsprintf(&weight_device, "%d:%d %d", + tmp[i].major, + tmp[i].minor, + tmp[i].weight); + if (weight_device) { + rc = virCgroupSetBlkioWeightDevice(group, weight_device); + VIR_FREE(weight_device); + weight_device = NULL; + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; + break; + } + } + } + VIR_FREE(tmp); } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5913,6 +6042,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; + } + VIR_FREE(persistentDef->blkio.weight_devices); + persistentDef->blkio.weight_devices = tmp; + persistentDef->blkio.ndevices = ndevices; } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -5946,9 +6090,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); @@ -5974,6 +6120,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; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE; @@ -6012,6 +6160,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < *nparams; i++) { + char *weight_device; virTypedParameterPtr param = ¶ms[i]; val = 0; param->value.ui = 0; @@ -6032,6 +6181,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; @@ -6055,6 +6221,25 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, param->value.ui = persistentDef->blkio.weight; break; + case 1: /* blkio.weight_device */ + if (typed_string) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + for (i = 0; i < persistentDef->blkio.ndevices; i++) { + virBufferAsprintf(&buf, "%s %d\n", + persistentDef->blkio.weight_devices[i].path, + persistentDef->blkio.weight_devices[i].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 7b0533d..e79d9ae 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -4616,6 +4616,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 major:minor,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")}, @@ -4626,6 +4628,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; @@ -4650,6 +4653,8 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) flags |= VIR_DOMAIN_AFFECT_LIVE; } + flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -4670,12 +4675,28 @@ 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. */ if (virDomainGetBlkioParameters(dom, NULL, &nparams, flags) != 0) { - vshError(ctl, "%s", - _("Unable to get number of blkio parameters")); - goto cleanup; + 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 (nparams == 0) { @@ -4717,6 +4738,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"); } @@ -4737,11 +4762,24 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); weight = 0; } + + if (weight_device) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&buf, "%s", weight_device); + temp->value.s = virBufferContentAndReset(&buf); + temp->type = VIR_TYPED_PARAM_STRING; + strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE, + sizeof(temp->field)); + } + } + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + flags &= ~VIR_DOMAIN_TYPED_STRING_OKAY; + 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 086fe93..c0b2af0 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1000,12 +1000,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/patch,weight;/device/patch,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 09/23/2011 12:40 AM, Hu Tao wrote:
This patch adds a parameter --weight-device to virsh command blkiotune for setting/getting blkio.weight_device. --- daemon/remote.c | 5 + include/libvirt/libvirt.h.in | 9 ++ src/conf/domain_conf.c | 114 ++++++++++++++++++++++++- src/conf/domain_conf.h | 16 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 22 +++++ src/qemu/qemu_driver.c | 191 +++++++++++++++++++++++++++++++++++++++++- src/util/cgroup.c | 33 +++++++ src/util/cgroup.h | 3 + tools/virsh.c | 52 ++++++++++-- tools/virsh.pod | 5 +- 11 files changed, 437 insertions(+), 14 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 82ee13b..bee1b94 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1566,6 +1566,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, int nparams = args->nparams; unsigned int flags; int rv = -1; + int i; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client);
@@ -1610,6 +1611,10 @@ success: cleanup: if (rv< 0) virNetMessageSaveError(rerr); + for (i = 0; i< nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_STRING) + VIR_FREE(params[i].value.s); + } VIR_FREE(params);
This for loop seems like something that will be done frequently enough that it might be worth factoring it into a util file for managing virTypedParameters. Something like: virTypedParameterFree(params).
if (dom) virDomainFree(dom); diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 448a0e7..a1f2c98 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1139,6 +1139,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. + */
Also mention that this name refers to a VIR_TYPED_PARAMETER_STRING.
+/** + * virDomainBlkioWeightDeviceParseXML + * + * this function parses a XML node: + * + *<device> + *<path>/fully/qulaified/device/path</path>
s/qulaified/qualified/
+ *<weight>weight</weight> + *</device> + * + * and fills a virBlkioWeightDevice struct. + */ +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root, + virBlkioWeightDevicePtr dw) +{ + char *c; + struct stat s; + xmlNodePtr node; + + if (!dw) + return -1; + + node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + c = (char *)xmlNodeGetContent(node); + if (!c) + return -1; + if (stat(c,&s) == -1) + return -1;
This stat() ties the parse to the existence of the node. But that seems wrong - it should be possible to define a domain with XML that refers to a node that does not yet exist, provided that the node later exists at the time we try to start the domain.
+ if ((s.st_mode& S_IFMT) == S_IFBLK) { + dw->major = major(s.st_rdev); + dw->minor = minor(s.st_rdev);
Also, breaking the parse into major/minor this early makes the result unmigratable, since we can't guarantee major/minor stability across hosts. I think you have to store the name, not the major/minor, here in the domain_conf representation of the device weight, and only convert to major/minor in the hypervisor that is actually enforcing the limits.
+ } else + return -1; + dw->path = (char *)xmlNodeGetContent(node); + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + dw->weight = atoi(c);
atoi is unsafe. It cannot detect errors. You should use virStrToLong_i (or similar) instead.
@@ -10499,10 +10591,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",
(Stupid Thunderbird for munging whitespace). This will conflict with my patch series for indenting <domainsnapshot>. Shouldn't be too hard to figure out, but it boils down to who gets acked first.
+++ b/src/qemu/qemu_driver.c @@ -44,6 +44,7 @@ #include<sys/ioctl.h> #include<sys/un.h> #include<byteswap.h> +#include<ctype.h>
This should use "c-ctype.h", not <ctype.h>.
+/* 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) +{ + struct stat s; + const char *temp; + int nDevice = 0; + int i, len; + virBlkioWeightDevicePtr result = NULL; + + if (!dw) + return -1; + if (*dw) + return -1;
This returns -1 without an error, but you later return -1 after reporting OOM error, so the caller can't tell things apart. That means you need to report an error here. On the other hand, since this function is static, you could audit that all callers pass correct parameters in the first place, and just omit this validation step.
+ + temp = weightDeviceStr; + while (temp) { + temp = strchr(temp, ';');
What happens if the absolute path of a device contains a literal ';'? I guess we don't anticipate that happening.
+ i = 0; + temp = weightDeviceStr; + while (temp&& i< nDevice) { + const char *p = temp; + + /* device path */ + + while (*p != ','&& *p != '\0') + ++p;
strchr(p,',') would be faster.
+ if (*p == '\0') + goto fail; + len = p - temp + 1; + if (VIR_ALLOC_N(result[i].path, len)< 0) + goto fail; + memcpy(result[i].path, temp, len - 1);
strndup() would be better than VIR_ALLOC_N and memcpy.
+ result[i].path[len - 1] = '\0'; + + if (stat(result[i].path,&s) == -1) { + VIR_FREE(result[i].path); + goto fail; + } + if ((s.st_mode& S_IFMT) != S_IFBLK) { + VIR_FREE(result[i].path); + goto fail; + } + result[i].major = major(s.st_rdev); + result[i].minor = minor(s.st_rdev);
major() and minor() are non-portable; this code may need to be conditionally compiled for non-Linux platforms. See how cgroup does conditional compilation before accessing major and minor numbers.
+ + /* weight */ + + temp = p + 1; + if (!temp) + goto fail; + + p = temp; + while (isdigit(*p)&& ++p);
s/isdigit/c_isdigit/
+ if (!p || (*p != ';'&& *p != '\0')) + goto fail; + + result[i].weight = atoi(temp);
No atoi. Use virStrToLong_i or similar.
static int qemuDomainSetBlkioParameters(virDomainPtr dom, virTypedParameterPtr params, int nparams, @@ -5860,10 +5951,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, ret = 0; if (flags& VIR_DOMAIN_AFFECT_LIVE) {
You forgot to update the virCheckFlags to allow the new VIR_DOMAIN_TYPED_STRING_OKAY flag. (Actually, I'm starting to think it might be better to name that flag VIR_TYPED_PARAM_STRING_OKAY, since it only makes sense with any API that uses virTypedParameter.
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'")); @@ -5884,6 +5975,44 @@ 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 tmp = NULL; + if (param->type != VIR_TYPED_PARAM_STRING) {
Here, you should also fail if flags does not include the OKAY flag, as that indicates a client that isn't properly using the API.
+ for (i = 0; i< ndevices; i++) { + char *weight_device = NULL; + + virAsprintf(&weight_device, "%d:%d %d", + tmp[i].major, + tmp[i].minor, + tmp[i].weight); + if (weight_device) { + rc = virCgroupSetBlkioWeightDevice(group, weight_device); + VIR_FREE(weight_device); + weight_device = NULL; + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set blkio weight_device tunable")); + ret = -1; + break; + } + }
You should report OOM errors if allocating weight_device failed, rather than silently ignoring them.
@@ -6012,6 +6160,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom,
if (flags& VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i< *nparams; i++) { + char *weight_device; virTypedParameterPtr param =¶ms[i]; val = 0; param->value.ui = 0;
If someone calls GetBlkioParameters with a size of 0 but without the _OKAY flag, to learn how large to allocate the array, then you need to return 1, not 2. I don't see that in your patch (I only see where if they allocate 2, but didn't pass _OKAY, that you don't populate the second entry).
+++ b/tools/virsh.c @@ -4616,6 +4616,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 major:minor,weight")},
s/per device/per-device/ Do not expose major:minor,weight through virsh. Rather, you should match the xml, and expose this via /path/to/device,weight (you got it right in virsh.pod).
+ flags |= VIR_DOMAIN_TYPED_STRING_OKAY; + if (!vshConnectionUsability(ctl, ctl->conn)) return false;
@@ -4670,12 +4675,28 @@ 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. */
Won't work as written. nparams will be -1, not 0, if the old server didn't understand the _OKAY flag. Also, you should check last_error->code, and only try the fallback if the error is VIR_ERR_INVALID_ARG (any other error is fatal without having to try the fallback).
if (virDomainGetBlkioParameters(dom, NULL,&nparams, flags) != 0) { - vshError(ctl, "%s", - _("Unable to get number of blkio parameters")); - goto cleanup; + 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 (nparams == 0) { @@ -4717,6 +4738,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"); } @@ -4737,11 +4762,24 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) sizeof(temp->field)); weight = 0; } + + if (weight_device) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&buf, "%s", weight_device); + temp->value.s = virBufferContentAndReset(&buf);
Why not just: temp->value.s = weight_device, instead of going through a virBuffer?
+ temp->type = VIR_TYPED_PARAM_STRING; + strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT_DEVICE, + sizeof(temp->field)); + } + } + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) {
You need to fix the logic to pass _OKAY if weight_device was specified. Also, I think your logic is not quite right in that you pre-set flags to include _OKAY even if you are not passing weight_device, which will cause needless problems when talking to an older server if you aren't even going to be changing device weight.
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/patch,weight;/device/patch,weight.
s/patch/path/g -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Hu Tao