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

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);