On 11/08/2011 05:43 AM, Stefan Berger wrote:
> On 11/08/2011 06:00 AM, Hu Tao wrote:
>> This adds per-device weights to<blkiotune>. Note that the
>> cgroups implementation only supports weights per block device,
>> and not per-file within the device; hence this option must be
>> global to the domain definition rather than tied to individual
>> <device>/<disk> entries:
>>
>> <domain ...>
>> <blkiotune>
>> <device>
>> <path>/path/to/block</path>
>> <weight>1000</weight>
>> </device>
>> </blkiotune>
>> ...
>>
>> This patch also adds a parameter --device-weight to virsh command
>> blkiotune for setting/getting blkiotune.weight_device for any
>> hypervisor that supports it. All<device> entries under
>> <blkiotune> are concatenated into a single string attribute under
>> virDomain{Get,Set}BlkioParameters, named "device_weight".
>>
>>
>> +/**
>> + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT:
>> + *
>> + * Macro for the blkio tunable weight_device: it represents the
>> + * per-device weight, as a string. The string is parsed as a
>> + * series of /path/to/device,weight elements, separated by ';'.
It looks like the rest of the code swapped over to "separated by ','",
so I made that tweak.
>> + * This function returns a string representing device weights that is
>> + * suitable for writing to /cgroup/blkio/blkio.weight_device, given
>> + * a list of per-device weights.
>> + */
>> +#if defined(major)&& defined(minor)
>> +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices,
>> + int ndevices,
>> + char **result)
>> +{
>> + int i;
>> + struct stat s;
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> + for (i = 0; i< ndevices; i++) {
>> + if (stat(devices[i].path,&s) == -1)
>> + return -1;
>> + if ((s.st_mode& S_IFMT) != S_IFBLK)
>> + return -1;
>> + virBufferAsprintf(&buf, "%d:%d %d\n",
>> + major(s.st_rdev),
>> + minor(s.st_rdev),
>> + devices[i].weight);
>> + }
>> +
>> + if (virBufferError(&buf)) {
>> + virBufferFreeAndReset(&buf);
>> + return -1;
>> + }
>> +
>> + *result = virBufferContentAndReset(&buf);
>> + return 0;
You fail for more than one reason, but don't output a failure log
message, which means the caller has to do it and can't guess why things
failed (OOM? not a block device?). I don't see any callers in this
patch, so I checked 5/5; there, both callers just reported an internal
error stating the string could not be parsed. But internal error for a
user-visible message is rather harsh, so I think this is worth improving.
>> +}
>> +#else
>> +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices
>> ATTRIBUTE_UNUSED,
>> + int ndevices ATTRIBUTE_UNUSED,
>> + char **result ATTRIBUTE_UNUSED)
>> +{
>> + return -1;
And if we improve the error reporting above, then we also have to report
an error here.
>> +static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root,
>> + virBlkioDeviceWeightPtr dw)
>> +{
>> + char *c;
>> + xmlNodePtr node;
>> +
>> + if (!dw)
>> + return -1;
Dead check - this is a static function, and we know the caller gives us
valid inputs.
>> + node = root->children;
>> + while (node) {
>> + if (node->type == XML_ELEMENT_NODE) {
>> + if (xmlStrEqual(node->name, BAD_CAST "path")) {
>> + dw->path = (char *)xmlNodeGetContent(node);
Memory leak; if the user (mistakenly) has more than one <path>
subelement, then you are blindly overwriting dw->path on the second
instance of path.
>> + } else if (xmlStrEqual(node->name, BAD_CAST "weight")) {
>> + c = (char *)xmlNodeGetContent(node);
>> + if (virStrToLong_ui(c, NULL, 10,&dw->weight)< 0) {
>> + VIR_FREE(c);
>> + VIR_FREE(dw->path);
>> + return -1;
Missing error reporting - either we must report the error here or in the
caller (I elected for here, to match precedence with other functions
used by the caller).
>> + }
>> + VIR_FREE(c);
>> + }
>> + }
>> + node = node->next;
>> + }
You never validated that path was a mandatory element.
>> @@ -6711,6 +6813,24 @@ static virDomainDefPtr
>> virDomainDefParseXML(virCapsPtr caps,
>> &def->blkio.weight)< 0)
>> def->blkio.weight = 0;
>>
>> + n = virXPathNodeSet("./blkiotune/device", ctxt,&nodes);
>> + if (n> 0) {
Missing an OOM memory check.
>> + if (VIR_ALLOC_N(def->blkio.devices, n)< 0) {
>> + virReportOOMError();
>> + goto error;
>> + }
>> +
>> + for (i = 0; i< n; i++) {
>> + if (virDomainBlkioDeviceWeightParseXML(nodes[i],
>> +&def->blkio.devices[i])< 0) {
>> + virBlkioDeviceWeightArrayClear(def->blkio.devices, i);
Redundant, if you had been updating def->blkio.ndevices as you go, for
consistency with some of the other clients of virXPathNodeSet in this
method.
>> +++ b/tools/virsh.c
>> @@ -4648,6 +4648,8 @@ static const vshCmdOptDef opts_blkiotune[] = {
>> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or
>> uuid")},
>> {"weight", VSH_OT_INT, VSH_OFLAG_NONE,
>> N_("IO Weight in range [100, 1000]")},
>> + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE,
>> + N_("per-device IO Weights, in the form of
>> /path/to/device,weight;...")},
Another place where we want to use ',', not ';'.
>> @@ -4749,6 +4762,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
>> vshPrint(ctl, "%-15s: %d\n", params[i].field,
>> params[i].value.b);
>> break;
>> + case VIR_TYPED_PARAM_STRING:
>> + vshPrint(ctl, "%-15s: %s\n", params[i].field,
>> + params[i].value.s);
>> + break;
>> default:
>> vshPrint(ctl, "unimplemented blkio parameter
>> type\n");
>> }
Memory leak - we aren't calling virTypedParameterArrayClear to free up
any returned strings that we just printed. But we can't blindly call it
in the cleanup: label of the function, because...
>> @@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd *
>> cmd)
>>
>> if (weight) {
>> temp->value.ui = weight;
>> - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT,
>> - sizeof(temp->field));
>> + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT,
>> + sizeof(temp->field))) {
>> + virTypedParameterArrayClear(params, nparams);
>> + ret = false;
>> + goto cleanup;
>> + }
>> weight = 0;
>> }
>> +
>> + if (device_weight) {
>> + /* Safe to cast away const here. */
>> + temp->value.s = (char *)device_weight;
...here we are not storing malloc'd memory there in the first place.
Using a judicious strdup simplifies the code.
>> + temp->type = VIR_TYPED_PARAM_STRING;
>> + if (!virStrcpy(temp->field,
>> VIR_DOMAIN_BLKIO_DEVICE_WEIGHT,
>> + sizeof(temp->field))) {
>> + virTypedParameterArrayClear(params, nparams);
>> + ret = false;
ret is already false, we can skip this assignment.
>> + goto cleanup;
>> + }
>> + }
>> }
>> - if (virDomainSetBlkioParameters(dom, params, nparams, flags)
>> != 0)
>> + ret = true;
>> + if (virDomainSetBlkioParameters(dom, params, nparams, flags)
>> != 0) {
Pre-existing, but as long as we are touching this code, it's better to
check for < 0 instead of != 0 for errors on libvirt calls.
>> +B<device-weights> is a single string listing one or more device/weight
>> +pairs, in the format of /path/to/device,weight;/path/to/device,weight.
Another comma.
>> +
>> If I<--live> is specified, affect a running guest.
>> If I<--config> is specified, affect the next boot of a persistent
>> guest.
>> If I<--current> is specified, affect the current guest state.
> ACK
I won't push this until I've tweaked and tested patch 5/5, but here's
what I plan on squashing in provided testing goes well.
diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in
index 0fd9302..ff4f51b 100644
--- i/include/libvirt/libvirt.h.in
+++ w/include/libvirt/libvirt.h.in
@@ -1241,7 +1241,7 @@ char *
virDomainGetSchedulerType(virDomainPtr domain,
*
* Macro for the blkio tunable weight_device: it represents the
* per-device weight, as a string. The string is parsed as a
- * series of /path/to/device,weight elements, separated by ';'.
+ * series of /path/to/device,weight elements, separated by ','.
*/
#define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight"
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index e4ae64c..8ac8d6f 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -604,8 +604,9 @@ VIR_ENUM_IMPL(virDomainStartupPolicy,
VIR_DOMAIN_STARTUP_POLICY_LAST,
#define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE
-void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights,
- int ndevices)
+void
+virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights,
+ int ndevices)
{
int i;
@@ -618,22 +619,26 @@ void
virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights,
*
* This function returns a string representing device weights that is
* suitable for writing to /cgroup/blkio/blkio.weight_device, given
- * a list of per-device weights.
+ * a list of per-device weights, or reports an error on failure.
*/
#if defined(major) && defined(minor)
-int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices,
- int ndevices,
- char **result)
+int
+virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices,
+ int ndevices,
+ char **result)
{
int i;
struct stat s;
virBuffer buf = VIR_BUFFER_INITIALIZER;
for (i = 0; i < ndevices; i++) {
- if (stat(devices[i].path, &s) == -1)
- return -1;
- if ((s.st_mode & S_IFMT) != S_IFBLK)
+ if (stat(devices[i].path, &s) == -1 ||
+ (s.st_mode & S_IFMT) != S_IFBLK) {
+ virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("path '%s' must be a block device"),
+ devices[i].path);
return -1;
+ }
virBufferAsprintf(&buf, "%d:%d %d\n",
major(s.st_rdev),
minor(s.st_rdev),
@@ -641,6 +646,7 @@ int
virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices,
}
if (virBufferError(&buf)) {
+ virReportOOMError();
virBufferFreeAndReset(&buf);
return -1;
}
@@ -649,10 +655,13 @@ int
virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices,
return 0;
}
#else
-int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices
ATTRIBUTE_UNUSED,
- int ndevices ATTRIBUTE_UNUSED,
- char **result ATTRIBUTE_UNUSED)
+int
+virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED,
+ int ndevices ATTRIBUTE_UNUSED,
+ char **result ATTRIBUTE_UNUSED)
{
+ virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("this binary lacks per-device weight support"));
return -1;
}
#endif
@@ -669,23 +678,24 @@ int
virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED,
*
* and fills a virBlkioDeviceWeight struct.
*/
-static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root,
- virBlkioDeviceWeightPtr dw)
+static int
+virDomainBlkioDeviceWeightParseXML(xmlNodePtr root,
+ virBlkioDeviceWeightPtr dw)
{
char *c;
xmlNodePtr node;
- if (!dw)
- return -1;
-
node = root->children;
while (node) {
if (node->type == XML_ELEMENT_NODE) {
- if (xmlStrEqual(node->name, BAD_CAST "path")) {
+ if (xmlStrEqual(node->name, BAD_CAST "path") && !dw->path) {
dw->path = (char *)xmlNodeGetContent(node);
} else if (xmlStrEqual(node->name, BAD_CAST "weight")) {
c = (char *)xmlNodeGetContent(node);
if (virStrToLong_ui(c, NULL, 10, &dw->weight) < 0) {
+ virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("could not parse weight %s"),
+ c);
VIR_FREE(c);
VIR_FREE(dw->path);
return -1;
@@ -695,6 +705,11 @@ static int
virDomainBlkioDeviceWeightParseXML(xmlNodePtr root,
}
node = node->next;
}
+ if (!dw->path) {
+ virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("missing per-device path"));
+ return -1;
+ }
return 0;
}
@@ -6813,23 +6828,21 @@ static virDomainDefPtr
virDomainDefParseXML(virCapsPtr caps,
&def->blkio.weight) < 0)
def->blkio.weight = 0;
- n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes);
- if (n > 0) {
- if (VIR_ALLOC_N(def->blkio.devices, n) < 0) {
- virReportOOMError();
- goto error;
- }
+ if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("cannot extract blkiotune nodes"));
+ goto error;
+ }
+ if (n && VIR_ALLOC_N(def->blkio.devices, n) < 0)
+ goto no_memory;
- for (i = 0; i < n; i++) {
- if (virDomainBlkioDeviceWeightParseXML(nodes[i],
-
&def->blkio.devices[i]) < 0) {
- virBlkioDeviceWeightArrayClear(def->blkio.devices, i);
- goto error;
- }
- }
- def->blkio.ndevices = n;
- VIR_FREE(nodes);
+ for (i = 0; i < n; i++) {
+ if (virDomainBlkioDeviceWeightParseXML(nodes[i],
+ &def->blkio.devices[i]) < 0)
+ goto error;
+ def->blkio.ndevices++;
}
+ VIR_FREE(nodes);
/* Extract other memory tunables */
if (virXPathULong("string(./memtune/hard_limit)", ctxt,
diff --git i/tools/virsh.c w/tools/virsh.c
index 4060e3d..30504be 100644
--- i/tools/virsh.c
+++ w/tools/virsh.c
@@ -4649,7 +4649,7 @@ static const vshCmdOptDef opts_blkiotune[] = {
{"weight", VSH_OT_INT, VSH_OFLAG_NONE,
N_("IO Weight in range [100, 1000]")},
{"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE,
- N_("per-device IO Weights, in the form of
/path/to/device,weight;...")},
+ N_("per-device IO Weights, in the form of
/path/to/device,weight,...")},
{"config", VSH_OT_BOOL, 0, N_("affect next boot")},
{"live", VSH_OT_BOOL, 0, N_("affect running domain")},
{"current", VSH_OT_BOOL, 0, N_("affect current domain")},
@@ -4770,8 +4770,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
vshPrint(ctl, "unimplemented blkio parameter type\n");
}
}
-
- ret = true;
} else {
/* set the blkio parameters */
params = vshCalloc(ctl, nparams, sizeof(*params));
@@ -4783,34 +4781,29 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
if (weight) {
temp->value.ui = weight;
if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT,
- sizeof(temp->field))) {
- virTypedParameterArrayClear(params, nparams);
- ret = false;
+ sizeof(temp->field)))
goto cleanup;
- }
- weight = 0;
}
if (device_weight) {
- /* Safe to cast away const here. */
- temp->value.s = (char *)device_weight;
+ temp->value.s = vshStrdup(ctl, device_weight);
temp->type = VIR_TYPED_PARAM_STRING;
if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT,
- sizeof(temp->field))) {
- virTypedParameterArrayClear(params, nparams);
- ret = false;
+ sizeof(temp->field)))
goto cleanup;
- }
}
}
- ret = true;
- if (virDomainSetBlkioParameters(dom, params, nparams, flags) !=
0) {
+
+ if (virDomainSetBlkioParameters(dom, params, nparams, flags) < 0) {
vshError(ctl, "%s", _("Unable to change blkio parameters"));
- ret = false;
+ goto cleanup;
}
}
+ ret = true;
+
cleanup:
+ virTypedParameterArrayClear(params, nparams);
VIR_FREE(params);
virDomainFree(dom);
return ret;
diff --git i/tools/virsh.pod w/tools/virsh.pod
index a117e84..70e299f 100644
--- i/tools/virsh.pod
+++ w/tools/virsh.pod
@@ -1040,7 +1040,7 @@ Display or set the blkio parameters. QEMU/KVM
supports I<--weight>.
I<--weight> is in range [100, 1000].
B<device-weights> is a single string listing one or more device/weight
-pairs, in the format of /path/to/device,weight;/path/to/device,weight.
+pairs, in the format of /path/to/device,weight,/path/to/device,weight.
If I<--live> is specified, affect a running guest.
If I<--config> is specified, affect the next boot of a persistent guest.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list