[libvirt] [PATCH] cgroup:fix bug to keep --device-weights value persistent

[Eric Blake]

On 02/02/2012 04:57 AM, Guannan Ren wrote:
>     src/qemu/qemu_driver.c
>     When run "virsh blkiotune dom --device-weights /dev/sda,400 --config"
>     it couldn't be persistent after dom restart.
>     The patch fix it.
> 
> ---
>  src/qemu/qemu_driver.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d66140b..1a53088 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5975,9 +5975,13 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size,
>                  virReportOOMError();
>                  return -1;
>              }
> -            (*def)[*def_size - 1].path = dw->path;
> +            (*def)[*def_size - 1].path = strdup(dw->path);

This much indirection makes life harder.  Can we declare an intermediate
variable of the right type, and initialize it with &(*def)[*def_size -
1], then use mydef->path and such through the rest of the code?  But
that's an independent comment about the code.

At any rate, the strdup here would make sense, if we were feeding the
temporary array that supplied dw into two different *def pointers.  But
in reality, the caller is creating the temporary array twice, so this
feels like it is just churn.

> +            if (!(*def)[*def_size - 1].path) {
> +                virReportOOMError();
> +                return -1;
> +            }
> +
>              (*def)[*def_size - 1].weight = dw->weight;
> -            dw->path = NULL;
>          }
>      }
>  
> @@ -5985,6 +5989,46 @@ qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t *def_size,
>  }
>  
>  static int
> +qemuDomainiDefineDeviceWeights(virDomainDefPtr persistentDef,

s/DomainiDefine/DomainDefine/ throughout the patch.

Actually, this function looks like it is reinventing
qemuDomainMergeDeviceWeights.

> @@ -6116,6 +6160,11 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
>                      ret = -1;
>                      continue;
>                  }
> +                if (qemuDomainiDefineDeviceWeights(persistentDef,
> +                                                   devices,
> +                                                   ndevices) < 0)
> +                    ret = -1;
> +

I'm not sure we need this; instead,

>                  if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices,
>                                                   &vm->def->blkio.ndevices,

Isn't the real bug that we are calling MergeDeviceWeights on vm->def
instead of on persistentDef?  Does this simpler patch do the trick?  (I
should probably split it into two pathes - the first hunk is cosmetic,
the second fixes the bug).

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 3f940e8..06b30be 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -5975,35 +5975,40 @@ cleanup:
     return -1;
 }

-/* Modify def to reflect all device weight changes described in tmp.  */
+/* Modify dest_array to reflect all device weight changes described in
+ * src_array.  */
 static int
-qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *def, size_t
*def_size,
-                             virBlkioDeviceWeightPtr tmp, size_t tmp_size)
+qemuDomainMergeDeviceWeights(virBlkioDeviceWeightPtr *dest_array,
+                             size_t *dest_size,
+                             virBlkioDeviceWeightPtr src_array,
+                             size_t src_size)
 {
     int i, j;
-    virBlkioDeviceWeightPtr dw;
+    virBlkioDeviceWeightPtr dest, src;

-    for (i = 0; i < tmp_size; i++) {
+    for (i = 0; i < src_size; i++) {
         bool found = false;

-        dw = &tmp[i];
-        for (j = 0; j < *def_size; j++) {
-            if (STREQ(dw->path, (*def)[j].path)) {
+        src = &src_array[i];
+        for (j = 0; j < *dest_size; j++) {
+            dest = &(*dest_array)[j];
+            if (STREQ(src->path, dest->path)) {
                 found = true;
-                (*def)[j].weight = dw->weight;
+                dest->weight = src->weight;
                 break;
             }
         }
         if (!found) {
-            if (!dw->weight)
+            if (!src->weight)
                 continue;
-            if (VIR_EXPAND_N(*def, *def_size, 1) < 0) {
+            if (VIR_EXPAND_N(*dest_array, *dest_size, 1) < 0) {
                 virReportOOMError();
                 return -1;
             }
-            (*def)[*def_size - 1].path = dw->path;
-            (*def)[*def_size - 1].weight = dw->weight;
-            dw->path = NULL;
+            dest = &(*dest_array)[*dest_size - 1];
+            dest->path = src->path;
+            dest->weight = src->weight;
+            src->path = NULL;
         }
     }

@@ -6142,8 +6147,8 @@ qemuDomainSetBlkioParameters(virDomainPtr dom,
                     ret = -1;
                     continue;
                 }
-                if (qemuDomainMergeDeviceWeights(&vm->def->blkio.devices,
-                                                 &vm->def->blkio.ndevices,
+                if
(qemuDomainMergeDeviceWeights(&persistentDef->blkio.devices,
+
&persistentDef->blkio.ndevices,
                                                  devices, ndevices) < 0)
                     ret = -1;
                 virBlkioDeviceWeightArrayClear(devices, ndevices);


-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120207/05039d5a/attachment-0001.sig>