9 Feb
2018
9 Feb
'18
6:05 a.m.
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
9 Feb
9 Feb
7:44 a.m.
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)
4:01 p.m.
"Fabio M. Di Nitto" <fdinitto(a)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?
--
Regards,
Feri
10 Feb
10 Feb
7:32 a.m.
On 2/9/2018 5:01 PM, Ferenc Wágner wrote:
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.
Agreed here, we are on the same page.
Fabio
"Fabio M. Di Nitto"
<fdinitto(a)redhat.com> writes:
right.
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?
14 Feb
14 Feb
8:32 a.m.
"Fabio M. Di Nitto" <fdinitto(a)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.
--
Regards,
Feri
2502
days inactive
2507
days old
4 comments
3 participants
participants (3)
-
Fabio M. Di Nitto -
Jan Pokorný -
wferi＠niif.hu