[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/5] blkiotune: add interface for blkiotune.device_weight



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]