[libvirt] [PATCH v4 2/2] add interface for blkio.weight_device
Eric Blake
eblake at redhat.com
Tue Oct 25 23:50:09 UTC 2011
On 10/12/2011 03:26 AM, Hu Tao wrote:
> 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(-)
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.
>
> 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);
This should be squashed into 1/2.
> +/**
> + * 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"
> +
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 ','.
> +++ 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)
> +{
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.
> + 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);
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.
> +static int virDomainBlkioWeightDeviceParseXML(xmlNodePtr root,
> + virBlkioWeightDevicePtr dw)
> +{
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.
> 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);
> +
Here, I'm thinking its better to have a for loop, freeing each
weight_devices[i].
> @@ -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) {
The for loop on ndevices renders this NULL check redundant.
> + int i;
> +
> + for (i = 0; i< def->blkio.ndevices; i++) {
> + virBufferAsprintf(buf, "<device>\n");
virBufferAddLit is faster, when it works.
> + virBufferAsprintf(buf, "<path>%s</path>\n",
> + def->blkio.weight_devices[i].path);
Must be escaped, in case it contains characters that would interfere
with xml parsing.
> +
> +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;
size_t for array length
> + virBlkioWeightDevicePtr weight_devices;
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.
> +++ 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"
'make syntax-check' rejected this, since you didn't use any of the
functions in that header.
>
> +/* 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;
Make temp const char*, or remove const from weightDeviceStr, instead of
casting away const.
> +
> + 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;
Memory leak - if you parse the first path, but the second strndup fails,
then you leak result[0].path.
> @@ -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;
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
More information about the libvir-list
mailing list