[libvirt] [PATCH v4 4/6] qemu: Add bps_max and friends QMP suport

[Matthias Gatto]

On Wed, Oct 1, 2014 at 6:05 PM, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 30.09.2014 16:09, Matthias Gatto wrote:
>>
>> Detect if the the qemu binary currently in use suport the bps_max option,
>> If yes add it to the command, if not, just ignore the options.
>>
>> Signed-off-by: Matthias Gatto <matthias.gatto at outscale.com>
>> ---
>>   src/qemu/qemu_monitor_json.c | 59
>> +++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 48 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index a8759dd..bef7b5b 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -4029,6 +4029,12 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon,
>>   }
>>
>>
>> +#define GET_THROTTLE_STATS_OPTIONAL(FIELD, STORE)
>> \
>> +    if (virJSONValueObjectGetNumberUlong(inserted,
>> \
>> +                                         FIELD,
>> \
>> +                                         &reply->STORE) < 0) {
>> \
>> +        reply->STORE = 0;
>> \
>> +    }
>
>
> Well, this is going to be called if-and-only-if qemu supports the new flags.
> Shouldn't we error out here rather than silently hiding a bug?
>
>>   #define GET_THROTTLE_STATS(FIELD, STORE)
>> \
>>       if (virJSONValueObjectGetNumberUlong(inserted,
>> \
>>                                            FIELD,
>> \
>> @@ -4050,7 +4056,6 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
>> result,
>>       size_t i;
>>       bool found = false;
>>
>> -    (void)supportMaxOptions;
>>       io_throttle = virJSONValueObjectGet(result, "return");
>>
>>       if (!io_throttle || io_throttle->type != VIR_JSON_TYPE_ARRAY) {
>> @@ -4096,6 +4101,16 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
>> result,
>>           GET_THROTTLE_STATS("iops", total_iops_sec);
>>           GET_THROTTLE_STATS("iops_rd", read_iops_sec);
>>           GET_THROTTLE_STATS("iops_wr", write_iops_sec);
>> +        if (supportMaxOptions)
>> +        {
>> +            GET_THROTTLE_STATS_OPTIONAL("bps_max", total_bytes_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("bps_rd_max",
>> read_bytes_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("bps_wr_max",
>> write_bytes_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("iops_max", total_iops_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("iops_rd_max",
>> read_iops_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("iops_wr_max",
>> write_iops_sec_max);
>> +            GET_THROTTLE_STATS_OPTIONAL("iops_size", size_iops_sec);
>
>
> This should be GET_THROTTLE_STAT() then.
>
>
>> +        }
>>
>>           break;
>>       }
>> @@ -4112,6 +4127,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
>> result,
>>       return ret;
>>   }
>>   #undef GET_THROTTLE_STATS
>> +#undef GET_THROTTLE_STATS_OPTIONAL
>>
>>   int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
>>                                         const char *device,
>> @@ -4122,16 +4138,37 @@ int
>> qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
>>       virJSONValuePtr cmd = NULL;
>>       virJSONValuePtr result = NULL;
>>
>> -    (void)supportMaxOptions;
>> -    cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
>> -                                     "s:device", device,
>> -                                     "U:bps", info->total_bytes_sec,
>> -                                     "U:bps_rd", info->read_bytes_sec,
>> -                                     "U:bps_wr", info->write_bytes_sec,
>> -                                     "U:iops", info->total_iops_sec,
>> -                                     "U:iops_rd", info->read_iops_sec,
>> -                                     "U:iops_wr", info->write_iops_sec,
>> -                                     NULL);
>> +    if (supportMaxOptions)
>> +    {
>> +        cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
>> +                                         "s:device", device,
>> +                                         "U:bps", info->total_bytes_sec,
>> +                                         "U:bps_rd",
>> info->read_bytes_sec,
>> +                                         "U:bps_wr",
>> info->write_bytes_sec,
>> +                                         "U:iops", info->total_iops_sec,
>> +                                         "U:iops_rd",
>> info->read_iops_sec,
>> +                                         "U:iops_wr",
>> info->write_iops_sec,
>> +                                         "U:bps_max",
>> info->total_bytes_sec_max,
>> +                                         "U:bps_rd_max",
>> info->read_bytes_sec_max,
>> +                                         "U:bps_wr_max",
>> info->write_bytes_sec_max,
>> +                                         "U:iops_max",
>> info->total_iops_sec_max,
>> +                                         "U:iops_rd_max",
>> info->read_iops_sec_max,
>> +                                         "U:iops_wr_max",
>> info->write_iops_sec_max,
>> +                                         "U:iops_size",
>> info->size_iops_sec,
>> +                                         NULL);
>> +    }
>> +    else
>> +    {
>> +        cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
>> +                                         "s:device", device,
>> +                                         "U:bps", info->total_bytes_sec,
>> +                                         "U:bps_rd",
>> info->read_bytes_sec,
>> +                                         "U:bps_wr",
>> info->write_bytes_sec,
>> +                                         "U:iops", info->total_iops_sec,
>> +                                         "U:iops_rd",
>> info->read_iops_sec,
>> +                                         "U:iops_wr",
>> info->write_iops_sec,
>> +                                         NULL);
>> +    }
>>       if (!cmd)
>>           return -1;
>>
>>
>
> Also, 'make syntax-check' fails here.
>
> Michal
>

The problem with GET_THROTTLE_STATS is that bps_max is an optional
parameter for qemu, so even if qemu support these parameters, if they
are not set, virJSONValueObjectGetNumberUlong return an error when
trying to get them.
Thank you for the review BTW.