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

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

 



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@xxxxxxxxxx    +1-919-301-3266
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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]