[libvirt] [PATCH 1/2] virsh: check if domiftune parameters fit into UINT

[John Ferlan]

On 07/31/2014 09:51 AM, Ján Tomko wrote:
> On 07/31/2014 02:44 PM, John Ferlan wrote:
>> 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]...
>>
> 
> Well, even a value of UINT_MAX is too large for current 'tc' to handle, hence
> the second patch in this series.
> 

Understood, but even "some day" that could change... like you point out
below hopefully not any time soon!

So back to the original patch - I'm fine to ACK as is. It'd be "nice" to
have the attach-interface path do the same checks, but it's going to
fail anyway and the message from tc should point out the reason why as
of the second patch.

John
>> 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.
> 
> The problem is in the PARAM_UINT limit. We could add new parameters with
> PARAM_ULLONG types, but virsh would still need to handle older daemons which
> support UINT and newer daemons should be able to handle older clients. This
> seemed like too much work for values that can't be handled by 'tc' and I was
> hoping to postpone it for a decade or two :)
> 
>>
>> 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.
>>
> 
> Jan
>