On 2/9/2018 5:01 PM, Ferenc Wágner wrote:
"Fabio M. Di Nitto" fdinitto@redhat.com writes:
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).
The plugins do accept an integer compress level because that's what they get from libknet. And this is a good fit for most, but requires an arbitrary mapping for lzo2, which has no concept of compress level, only different compression algorithms, directly selectable by the user.
right.
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. 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.
While I can imagine it work, it still somehow feels conceptually backwards to me. What if we let a "bad" value through? The first send will fail, I guess. Is that really worse than filtering during configuration?
That is exactly why we should still try to compress a random 0 byte buffer with the value during knet_compress_config and verify if the value is accepted by the underneath library. The config change happen with a write lock and there is no traffic passing by at all.
so basically:
- normal operations, traffic is going - user calls knet_compress_config - internally: - traffic stops (this happens now as well, no changes here) - compress level validation API will try to compress a random static buffer to see if the value is accepted by the compression library - if not, return error and etc.. - if yes, move on and release the hell hounds :-)
In short I am suggesting to delegate the val_level to the external library. I don´t see how that´s worse than filtering during configuration tho and doesn´t affect any TX traffic.
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).
It's easy to do and maintain and saves a round trip to the fine documentation, so why not?
Agreed here, we are on the same page.
Fabio