Fix ZMQ_HEARTBEAT_TTL maximum value check #3135
Conversation
| @@ -665,8 +667,8 @@ int zmq::options_t::setsockopt (int option_, | |||
|
|
|||
| case ZMQ_HEARTBEAT_TTL: | |||
| // Convert this to deciseconds from milliseconds | |||
| value = value / 100; | |||
| if (is_int && value >= 0 && value <= 6553) { | |||
There was a problem hiding this comment.
I'm not sure this was a typo? Right now the max TTL is one hour and a half, this would make it 18 hours which is a bit much
There was a problem hiding this comment.
Ok... that's really in the spec... But it is a very strange value, isn't it? ;)
Anyway, I will change it back to the previous value, but it makes sense to add the tests, so I keep the PR open and fix.
There was a problem hiding this comment.
It's very very strange :-)
The spec is in DRAFT form, so feel free to amend it to something less strange if you want to
There was a problem hiding this comment.
Oh, I checked it again... and now I understand, but it is even a bit weirder:
First, the spec says:
The ping-ttl is a 16-bit unsigned integer in network order that MAY be zero or MAY contain a time-to-live measured in tenths of seconds
This would allow values up to UINT16_MAX==65535 deciseconds, as my PR implements.
Then it says:
The maximum TTL is 6553 seconds
This would allow values up to 65530 deciseconds
So UINT16_MAX is almost correct according to the spec, in fact it were UINT16_MAX - UINT16_MAX % 10.
However, I think this second sentence is not meant to restrict the value, but simply explain the consequence of having a 16 bit unsigned integer specifying deciseconds.
Maybe the wording in the spec should be changed to:
As a consequence, the maximum TTL is 6553.5 seconds
There was a problem hiding this comment.
right, that makes even more sense, feel free to send a pr to the rfc and I'll merge that too
There was a problem hiding this comment.
Ok, I will also add this to doc/zmq_setsockopt.txt
sigiesec
force-pushed
the
fix-heartbeat-ttl-max
branch
from
c16c331 to
c7632be
Compare
Solution: use UINT16_MAX
tests/test_heartbeats.cpp
Outdated
| @@ -366,6 +374,49 @@ DEFINE_TESTS (pull, push, ZMQ_PULL, ZMQ_PUSH) | |||
| DEFINE_TESTS (sub, pub, ZMQ_SUB, ZMQ_PUB) | |||
| DEFINE_TESTS (pair, pair, ZMQ_PAIR, ZMQ_PAIR) | |||
|
|
|||
| const int deciseconds_per_millisecond = 100; | |||
| // TODO the maximum value is not documented, should it? | |||
sigiesec
force-pushed
the
fix-heartbeat-ttl-max
branch
from
c7632be to
7637609
Compare
Solution: added tests
Solution: provide definition based on _UI16_MAX
Fixes https://sonarcloud.io/project/issues?id=zeromq-libzmq&issues=AWOS_NBc_Tl7jhO8m4MM&open=AWOS_NBc_Tl7jhO8m4MM en passant
Fixes #3136