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@xxxxxxxxxx> --- 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
