Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed restrictions on pre-bind only options #1332

Merged
merged 1 commit into from Jun 11, 2020

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Jun 9, 2020

This fix changes restrictions on options that are being taken into account only at the function that is called during binding - both self-binding done on an unbound socket used for srt_connect and the manual binding by srt_bind. When these options were altered between srt_bind and srt_connect the option setting would be ignored without making the user aware.

@ethouris ethouris self-assigned this Jun 9, 2020
@ethouris ethouris added [core] Area: Changes in SRT library core Impact: Significant Priority: High Type: Maintenance Work required to maintain or clean up the code labels Jun 9, 2020
@ethouris ethouris added this to Backlog in Development via automation Jun 9, 2020
@ethouris ethouris modified the milestones: v1.5.0, v1.5.0 - Sprint 16 Jun 9, 2020
@ethouris ethouris moved this from Backlog to Reviewer Approved in Development Jun 9, 2020
@ethouris ethouris moved this from Reviewer Approved to Needs Review in Development Jun 9, 2020
@maxsharabayko
Copy link
Collaborator

Why e.g. the latency is not allowed to be changed after a socket is bound to some IP?
I think this config can't be changed only if a socket is establishing a connection or already connected. What do you think?

@maxsharabayko maxsharabayko added this to In progress in GitHub Issues via automation Jun 9, 2020
@maxsharabayko maxsharabayko removed this from Needs Review in Development Jun 9, 2020
@maxsharabayko maxsharabayko moved this from In progress to Needs Review in GitHub Issues Jun 9, 2020
@ethouris
Copy link
Collaborator Author

ethouris commented Jun 9, 2020

Theoretically, but I'm simply not changing the internals here, but adjusting the API entries to the internals. This is a separation between the state and the options for those options that then undergo "negotiation" during the handshake. Example: the state of TLPKTDROP is notified as per agent and peer separately, although if it's not supported by the peer, then the state is not set to true even if the option dictates otherwise.

For options that are not mapped to the state negotiated during the handshake it doesn't matter. Important here is that the INIT/OPEN state distinction is the only way to make a border point between the option and the state. This is important for options that WISH to alter the state, but the real state change is in force only if the handshake process also confirms it.

@maxsharabayko
Copy link
Collaborator

Didn't get your point.
Why not just use the following condition like it is used for SRTO_FC?

if (m_bConnecting || m_bConnected)

@ethouris
Copy link
Collaborator Author

Again.

A socket to be connected passes the following transition:

INIT -> OPEN -> CONNECTING -> CONNECTED

If you don't bind it yourself, it will undergo self-binding, so it enters the srt_connect function in INIT state, and then transits to OPEN during self-binding, then to CONNECTING when it started the handshake. If you bind it yourself, that is call srt_bind, it will enter srt_connect function already in OPEN state, so self-binding procedure will be skipped.

Both self-binding and explicit binding functions will call CUDT::open and it will transit the socket from INIT to OPEN. This procedure contains rewriting the values from the variables keeping the values of the options into the variables that keep the state - which means, after transiting to OPEN state the change in pre-bind options is meaningless - it will no longer influence the state.

The fix is simple: all such options, which are being read-and-forgotten in CUDT::open function (in particular, the sub-call of CUDT::clearData) need that the socket be in INIT, not OPEN state.

Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This procedure contains rewriting the values from the variables keeping the values of the options into the variables that keep the state - which means, after transiting to OPEN state the change in pre-bind options is meaningless - it will no longer influence the state.

Ok, it makes sense.

What about other pre-binding options then? E.g. SRTO_RENDEZVOUS still checks for m_bConnecting or m_bConnected.
SRTO_KMPREANNOUNCE and SRTO_ENFORCEDENCRYPTION check only for m_bConnected.

Shouldn't they also check for m_bOpened then?

@ethouris
Copy link
Collaborator Author

Just take a look in the code as to how variables that are assigned to these options are used. None of the options you mentioned causes setting up the state that is not read after that. m_bRendezvous is still checked in CUDT::startConnect, m_bOPT_StrictEncryption is checked even later, during the handshake process.

It's about the options that are recorded in fields that are referred to only during opening and not read any time later.

@maxsharabayko
Copy link
Collaborator

I'm thinking if it should be reflected in the documentation then?

E.g. I am looking at SRTO_ENFORCEDENCRYPTION and see "binding - pre".
The same for SRTO_TSBPDMODE.
But one will fail if tried to be set after srt_bind(..), while another will succeed.

How does a user know the difference?

@ethouris
Copy link
Collaborator Author

There's not much we can do about it, unless we fix the internals. For now to be simple we should state that the users should set all PRE options, then possibly do bind, then connect. I don't think it matters for users if they could set some options before binding and some after binding.

GitHub Issues automation moved this from Needs Review to Reviewer Approved Jun 11, 2020
@maxsharabayko maxsharabayko merged commit a0994d1 into Haivision:master Jun 11, 2020
GitHub Issues automation moved this from Reviewer Approved to Done Jun 11, 2020
@mbakholdina mbakholdina removed this from the v1.5.0 - Sprint 16 milestone Oct 14, 2020
@mbakholdina mbakholdina added this to the v1.4.2 milestone Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Priority: High Type: Maintenance Work required to maintain or clean up the code
Projects
No open projects
GitHub Issues
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants