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

[Martin Kletzander]

On Thu, Jan 26, 2017 at 12:43:09PM +0100, Michal Privoznik wrote:
>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 :-)
>

OK, I did that now.

>Michal
-------------- 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/20170126/13780c47/attachment-0001.sig>