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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux