[libvirt] [PATCH 1/5] qemu: Only set group_name when actually requested

[Martin Kletzander]

On Wed, Jan 25, 2017 at 06:43:39AM -0500, John Ferlan wrote:
>
>
>On 01/25/2017 04:16 AM, Martin Kletzander wrote:
>> We were setting it based on whether it was supported and that lead to
>> setting it to NULL, which our JSON code caught.  However it ended up
>> producing the following results:
>>
>>  $ virsh blkdeviotune fedora vda --total-bytes-sec-max 2000
>>  error: Unable to change block I/O throttle
>>  error: internal error: argument key 'group' must not have null value
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 516a851d3d55..f45972e3c823 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17506,7 +17506,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>          qemuDomainObjEnterMonitor(driver, vm);
>>          ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
>>                                              &info, supportMaxOptions,
>> -                                            supportGroupNameOption,
>> +                                            set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME,
>>                                              supportMaxLengthOptions);
>>          if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>              ret = -1;
>>
>
>I believe this should be a change to qemuMonitorJSONSetBlockIoThrottle
>to use 'S' instead of 's' in the virJSONValueObjectAdd call.
>
>Been too long to remember why I went with 's' - although I wonder if it
>had to do with "unsetting" the value... I do believe I should have gone
>with my first instincts on this and should have made the design around
>allowing the user to define a group number to use and then convert that
>into a group name string with a static prefix and the number <sigh>.
>

The way other options (like _max options, or the _bytes,_iops) work like
this *because* they need to be set either all or none.  The group_name
can be set or not set by itself, no need for other options as there is
no clash if you just update that.  You also can't set it to what you got
from QEMU, because QEMU can change it automatically; if you have no
block I/O throttle set at all then there is no group, when you set
anything the group name defaults to the id of the device (that way it is
unique and it works like there is no group).  The same way if you then
want to reset it, without changing the group name, you can't set
everything to zeroes and then group name to the same string, it will be
reset anyway.  So group name should've had a different treatment than
the other options.

>ACK with that change.
>
>John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170125/107e97dc/attachment-0001.sig>