Hey guys,
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. 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?
Fabio
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?
"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.
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?
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?
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
"Fabio M. Di Nitto" fdinitto@redhat.com writes:
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.
Agreed. And keep the current logic in the lzo2 plugin, maybe adding some more logging. Even a full roundtrip into the compression library might be worth considering. I don't think we really need random data for that, though.
-
Fabio M. Di Nitto -
Jan Pokorný -
wferi＠niif.hu