On 09/02/18 07:05 +0100, Fabio M. Di Nitto wrote:
original discussion started here (for reference):
https://github.com/kronosnet/kronosnet/pull/128
int knet_handle_compress(knet_handle_t knet_h,
struct knet_handle_compress_cfg *knet_handle_compress_cfg);
Where knet_handle_compress_cfg contains int compress_level.
As you can see from libknet.h and relative code, we try (too hard) to
validate the values for compress_level.
Feri had a proper concern about applying incorrect filtering and I agree
that it is already becoming complex to maintain a matrix of valid
values, per plugin, per plugin-version, and whatever combination.
AFAIR all plugins accept an int value for compress_level so from that
perspective it shouldn´t be too much of a problem (aka no API/ABI changes).
Historically, the reason why I added the validation was lzo2, that
presents a rather unusual compress configuration vs all other plugins
(as you can see).
I think Feri´s suggestion to drop the filter completely might be too
much, instead, after thinking a bit, it might be perfectly reason to
change the internals of val_level to try and compress a small buffer to
test that the value is good.
Without deeper look, it sounds like responsibility separation went
wrong -- the only qualified units to know whether some compression
parameter is valid are likely just the plugins themselves and such
decision should be delegated there. The sketched approach is just
second guessing, invalid compression level may be mapped to "none"
under the hood and all may seem to be working fine, except when
it's not.
The val_level is used only at configuration time, so
it´s not too
expensive to compress one small data chunk for validation and it would
remove the whole hardcoded filter of different compress_levels.
For special plugins like lzo2, I think it´s important we improve the
logging at configuration time to make sure users at least warned of
supported values (as you can see each values map to a specific lzo2 API
call to compress).
wdyt?
--
Jan (Poki)