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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list