Re: [libvirt] [PATCH 1/2] virsh: check if domiftune parameters fit into UINT
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