[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