[libvirt] [PATCH] cgroup:fix bug to keep --device-weights value persistent
Guannan Ren
gren at redhat.com
Wed Feb 8 13:49:05 UTC 2012
On 02/08/2012 08:37 AM, Eric Blake wrote:
> 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);
>
>
Thanks, it is using duplicated function to do the job.
Your patch is perfect except the possible not correct
indentation in last couple of lines.
More information about the libvir-list
mailing list