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

[Michal Privoznik]

On 01/25/2017 03:18 PM, John Ferlan wrote:
> 
> 
> On 01/25/2017 08:55 AM, Martin Kletzander wrote:
>> 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.
>>
> 
> You have an ACK from Michal for the methodology you've chosen - that's
> fine - I just think it's cleaner to use the 'S' instead of 's'.

We can combine these two approaches, can't we? That way we are double
sure that the bug will not occur again :-)

Michal