Free-form text rejection message sent back to the caller #1170
Conversation
ethouris
added
Type: Enhancement
| // it's already done during handshake serialization. | ||
|
|
||
| // Now check the obligatory HS flags. HSX is required, KMX and CONFIG optional. | ||
| int ext_flags = SrtHSRequest::SRT_HSTYPE_HSFLAGS::unwrap(hs.m_iType); |
There was a problem hiding this comment.
This is just inverted order, while the size check is for not just the plain handshake, but + the HSREQ extension. Basic size doesn't have to be checked - it was done already when deserializing the handshake. But then the flag should be checked first because with the flag it should grant that it also contains appropriate extensions.
|
|
||
| if (contents.size() >= size_limit) | ||
| { | ||
| contents = "#!::="; // size error |
There was a problem hiding this comment.
Just came to my mind: probably this isn't possible here, as the size limit should be checked when setting streamid.
srtcore/handshake.h
Outdated
| @@ -227,23 +227,34 @@ enum UDTRequestType | |||
|
|
|||
| // Errors reported by the peer, also used as useless error codes | |||
There was a problem hiding this comment.
Errors reported by the peer, also used as useless error codes
"Useless" or "useful"?
There was a problem hiding this comment.
This comment comes from times when these codes were useless indeed. This can be now commented better.
srtcore/core.h
Outdated
| @@ -1530,7 +1532,10 @@ class CUDT | |||
|
|
|||
| int processData(CUnit* unit); | |||
| void processClose(); | |||
| SRT_REJECT_REASON processConnectRequest(const sockaddr_any& addr, CPacket& packet); | |||
|
|
|||
| /// Returns: URQ code, possibly containing reject reason | |||
There was a problem hiding this comment.
Doxygen style requires:
/// @returns URQ code, possibly containing reject reason
srtcore/core.cpp
Outdated
| int error = SRT_REJ_UNKNOWN; | ||
| string streaminfo_msg; | ||
| int result = s_UDTUnited.newConnection(m_SocketID, addr, packet, (hs), (error), (streaminfo_msg)); |
There was a problem hiding this comment.
Why do you get the contents of STREAM_ID. To put it into the response handshake?
How was it done before, if it was?
There was a problem hiding this comment.
It wasn't. That's a new thing here. Previously the failure response handshake contained no extensions. This is copying it back in order to pass it in case when the user has set some contents here.
There was a problem hiding this comment.
So this is something that we decided not to do at the moment.
There is no precedure for this defined yet.
srtcore/handshake.h
Outdated
| inline int RejectReasonForURQ(UDTRequestType req) | ||
| { | ||
| if (req < URQ_FAILURE_TYPES || req - URQ_FAILURE_TYPES >= SRT_REJ__SIZE) | ||
| if (req < URQ_FAILURE_TYPES) | ||
| return SRT_REJ_UNKNOWN; | ||
| return SRT_REJECT_REASON(req - URQ_FAILURE_TYPES); | ||
|
|
||
| if (req < URQ_SERVER_FAILURE_TYPES && req - URQ_FAILURE_TYPES >= SRT_REJ_E_SIZE) | ||
| return SRT_REJ_UNKNOWN; |
There was a problem hiding this comment.
I think the second if can be simplified to just:
if (req - URQ_FAILURE_TYPES >= SRT_REJ_E_SIZE)
return SRT_REJ_UNKNOWN;There was a problem hiding this comment.
Well, wrong. This will change the behavior when req > URQ_SERVER_FAILURE_TYPES. This condition is in order to embrace the range between SRT_REJ_E_SIZE and SRT_REJC_SERVER (for the value of req - URQ_FAILURE_TYPES).
Co-authored-by: stevomatthews <[email protected]>
Co-authored-by: stevomatthews <[email protected]>
srtcore/core.cpp
Outdated
| int CUDT::rejectReason(SRTSOCKET u, int value) | ||
| { | ||
| CUDTSocket* s = s_UDTUnited.locateSocket(u); | ||
| if (!s || !s->m_pUDT) | ||
| return APIError(MJ_NOTSUP, MN_SIDINVAL); | ||
|
|
||
| if (value < SRT_REJC_SERVER) | ||
| return APIError(MJ_NOTSUP, MN_INVAL); | ||
|
|
||
| s->m_pUDT->m_RejectReason = value; |
There was a problem hiding this comment.
SRT_REJC_SERVER = 1000 (see srt.h)
srt_setrejectreason(..) should return error if the value >=2000 is passed.
The values below are SRT internal.
if (value < SRT_REJC_SERVER) does not protect from this.
There was a problem hiding this comment.
Or I am probably confused by SRT API and SRT protocol error ranges. 🤔
There was a problem hiding this comment.
There is a confusion, so let me explain:
The rejection code is passed through the URQ field in the handshake. For this number the ranges are valid codes, below 1000. Since 1000 on there start values of rejection codes, system ones this time, and those are these collected in SRT_REJECT_REASON. The code is then 1000 + the reject reason. Therefore for this field the server and user codes will be shifted by 2000 and 3000 respectively.
Translation from URQ into the rejection code is URQ - 1000, however:
- Codes from the system region (0 to 999) result in the system error code, unless the value is outside the system rejection values, in which case it renders to UNKNOWN
- Otherwise pass the code as is, just doing URQ - 1000.
|
Do I understand this correctly? SRT Protocol Error ranges (transmitted in the "Handshake Type" field)
SRT API Error ranges
|
|
Yes, that's correct, I'd just add lacking parrts: For the handshake field, values between -3 and 1 are valid request types. Errors in general start with 1000. For the rejection code [0 - 1000] are reserved for system and values up to 15 are currently in use. |
ethouris
changed the title
The STREAMID extension field together with associated
SRTO_STREAMIDoption is reused for the purpose of returning the error message.USAGE:
The listener callback handler is allowed to set the streamid string either through
SRTO_STREAMIDoption orUDT::setstreamidextension function on the connecting socket passed to the handler. This very message will be then able to be extracted from the connector socket through the same option (orUDT::getstreamid).Note that on the caller side the user should get the STREAMID value. If this is not empty, it should contain a string message that the server has passed to the caller.
The
srt-test-liveapplication contains the example hook that simply rejects every connection with customizable error code and message. The caller side should display it.Addresses partially #1292