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

[John Ferlan]

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'.

>From the aspect of clearing after setting - right for live path that
wouldn't make sense, so no sense in allowing that modification anyway.

John