Re: [libvirt] [PATCH 5/5] qemu: Add better message for some invalid block I/O settings
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(a)redhat.com>
>> ---
>> 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.
>