Re: [libvirt] [PATCH 1/2] virsh: check if domiftune parameters fit into UINT
On 07/31/2014 06:00 AM, Ján Tomko wrote:
On 07/30/2014 10:34 PM, John Ferlan wrote:
>
>
> On 07/28/2014 09:30 AM, Ján Tomko wrote:
>> We parse the bandwidth rates as unsinged long long,
>> then try to fit them in VIR_TYPED_PARAM_UINT.
Just noticed this too - I see your fingers do the same as mine and typed
"unsinged" - that'd need to change to "unsigned"
>>
>> Report an error if they exceed UINT_MAX instead of
>> quietly using wrong values.
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1043735
>> ---
>> tools/virsh-domain.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index ad68aab..f7193cb 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -2686,6 +2686,14 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd)
>> vshError(ctl, _("inbound format is incorrect"));
>> goto cleanup;
>> }
>
> I think the parseRateStr() should be modified that way the
> attach-interface can also make use of this range check as well...
cmdAttachInterface is not limited by this range check - it transmits the value
via XML as a string. But I could move the error messages there and use a
different maximum for attach-interface.
After doing a bit of digging - it seems using attach-interface allows >
UINT_MAX for these values, while using domiftune limits one to UINT_MAX.
When first looking at the code I just figured since both use the same
function to parse the values and both are using virNetDevBandwidthRate
to store values, then both could have the same limits applied. Although
I understood it was outside the scope of the original bz which was just
for the domiftune. A side note - the man page or docs do not seem to
list a "max" value, although one would have a tough time imagining today
such a large value for a network especially in kb[/s]...
The qemu API qemuDomainSetInterfaceParameters() has a
virTypedParamsValidate() which should check values vs.
VIR_TYPED_PARAM_UINT; however, virNetDevBandwidthSet() handles ULL
values so one wonders if it's necessary to limit in domiftune. The
latter API is where the final size decision is determined.
Using ULL by virNetDevBandwidthRate was designed that way as the
original data structure was added in commit id '7373188'. The
attach-interface code was added in '7b2723c5'. The original domiftune
code was added afterwards as commit id 'b2310b29' and it seemed to
"limit" to VIR_TYPED_PARAM_UINT. Commit id '9b2d2446' adjusted things
to
use virTypedParamsAddUInt(), but still the UINT seems to be a "day 1"
type difference.
Whether domiftune should use ULL instead of UINT_MAX as the max is the
question that needs to be answered. Perhaps the "right" solution is to
modify the code to accept the ULL instead of limiting to UINT_MAX since
the underlying algorithms manage the data in ULL.
John