On 01/25/2017 09:07 AM, Martin Kletzander wrote: > On Wed, Jan 25, 2017 at 07:38:26AM -0500, John Ferlan wrote: >> >> >> On 01/25/2017 04:16 AM, Martin Kletzander wrote: >>> For example when both total_bytes_sec and total_bytes_sec_max are set, >>> but the former gets cleaned due to new call setting, let's say, >>> read_bytes_sec, we end up with this weird message for the command: >>> >>> $ virsh blkdeviotune fedora vda --read-bytes-sec 3000 >>> error: Unable to change block I/O throttle >>> error: unsupported configuration: value 'total_bytes_sec_max' cannot >>> be set if 'total_bytes_sec' is not set >>> >>> So let's make it more descriptive. This is how it looks after the >>> change: >>> >>> $ virsh blkdeviotune fedora vda --read-bytes-sec 3000 >>> error: Unable to change block I/O throttle >>> error: unsupported configuration: cannot reset 'total_bytes_sec' >>> when 'total_bytes_sec_max' is set >>> >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1344897 >>> >>> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> >>> --- >>> src/qemu/qemu_driver.c | 46 >>> +++++++++++++++++++++++++++++++--------------- >>> 1 file changed, 31 insertions(+), 15 deletions(-) >>> >> >> At least this is easier with those macros... I'm fine with the error >> message adjustments here - although I thought QEMU checked the various >> inconsistencies and messaged based on that (perhaps something I checked >> when using the qemu command line instead of via virsh - cannot recall >> now). >> > > It does, but we don't handle all types of errors very well. Also it's > better to error out earlier, what if some version of QEMU will set half > of the settings before failing? I recall running into "odd" errors while adding the _max changes, but since QEMU messaged them I think I felt at the time it would be better to punt and let QEMU handle it rather than checking in libvirt. I do agree though - I'd rather see libvirt code check for these types of errors, but I also know I've done that in other code and have had review comments that essentially said - let QEMU handle that and use the QEMU message. > > Anyway, QEMU checked that, but it ended up like this (can be > triggerredfor example by setting *bytes_sec to more than > *bytes_sec_max): > > $ virsh blkdeviotune vm2 vda --total-bytes-sec 3000 > error: Unable to change block I/O throttle > error: internal error: Unexpected error > > See the BZ for details. I didn't want to make the commit messages > longer than the (self-describing) patches themselves. I quickly scanned the bz... For some reason I thought though that I was getting at least some message back from qemu in 'later' libvirt code rather than that Unexpected error... as if some other patch had made sure to propagate the qemu message. John > >> ACK, >> >> John >> >> oh and before I forget... Could you please update: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1336564 >> >> to indicate which commit fixes the issues for group name. Thanks and >> sorry for the mess. >> -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
