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

Re: [libvirt] [PATCH v4 2/2] add interface for blkio.weight_device



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


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