compression plugin compress level validation

Jan Pokorný jpokorny at redhat.com
Fri Feb 9 08:44:58 CET 2018


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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.kronosnet.org/pipermail/devel/attachments/20180209/4189437d/attachment.pgp>


More information about the Devel mailing list