[libvirt] [PATCH] qemu: Yet another check for blkdeviotune values

[Martin Kletzander]

On Thu, Jun 09, 2016 at 10:56:01AM -0400, John Ferlan wrote:
>
>
>On 06/07/2016 05:26 PM, Martin Kletzander wrote:
>> If you want to set block device I/O tuning values that end with '_max'
>> and there is nothing else set, libvirt emits an error.  In particular:
>>
>>   error: internal error: Unexpected error
>>
>> That's an unknown error.  That is because *_max values depend on their
>> respective non-_max values.  QEMU even says that in the error message
>> sent as a response to the monitor command:
>>
>>   "error": {"class": "GenericError", "desc": "bps_max/iops_max require
>>   corresponding bps/iops values"}
>>
>> the problem was that we didn't know that and there was no check for it.
>> Adding such check makes sure that there will be less confused users.
>>
>> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> ---
>>  src/qemu/qemu_driver.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a7202ed3e3a1..d73238a66c66 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17391,6 +17391,25 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>          }
>>          if (!set_size_iops)
>>              info.size_iops_sec = oldinfo->size_iops_sec;
>> +
>> +#define CHECK_MAX(val)                                                  \
>> +        do {                                                            \
>> +            if (info.val##_max && !info.val) {                          \
>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,              \
>> +                               _("Value '%s' cannot be set if "         \
>> +                                 "'%s' is not set"),                    \
>> +                               #val "_max", #val);                      \
>> +                goto endjob;                                            \
>> +            }                                                           \
>> +        } while (false);
>> +
>> +        CHECK_MAX(total_bytes_sec);
>> +        CHECK_MAX(read_bytes_sec);
>> +        CHECK_MAX(write_bytes_sec);
>> +        CHECK_MAX(total_iops_sec);
>> +        CHECK_MAX(read_iops_sec);
>> +        CHECK_MAX(write_iops_sec);
>> +
>
>s/Value/value/  (NIT: and only since  you'll be in there)
>

More of a good point then a nit, actually.

>Couldn't decide if CONFIG_UNSUPPORTED was better than perhaps
>OPERATION_UNSUPPORTED which indicates that qemu refuses to accept just
>_max without non-_max
>

This was a copy from few lines above when we report CONFIG_UNSUPPORTED
for some values or older QEMU.

>Need to add:
>
>#undef CHECK_MAX
>

oh, forgot that completely

>ACK w/ that added.
>

Thanks, pushed.

>John
>
>>          qemuDomainObjEnterMonitor(driver, vm);
>>          ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
>>                                              &info, supportMaxOptions);
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160609/10e01b72/attachment-0001.sig>