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

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(a)redhat.com&gt; >>> --- >>> 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'.