@ovoshlook pushed 1 commit.

3f1c45c01fcf981eb949df48c0cb099d70aa4ba1 Merge branch 'master' into ovoshlook/register-auth

@ovoshlook pushed 1 commit.

c3dd758c47de901afd374487bf620e5a1f9aa3e3 tm: Do not discard late negative replies for forked REGISTER

linuxmaniac left a comment (kamailio/kamailio#4304)

Having 4 commits with no lack to resolve formatting for commit and code (profiling failed), is it better to create a new PR with proper changes in one commit, or is it minor? ( BTW, don't get why commit message formatting failed)

You just need to rebase and push force your branch

@ovoshlook pushed 1 commit.

9200b73df11d1f40412f08760c311e4858e09d39 tm: Do not discard late negative replies for forked REGISTER

ovoshlook left a comment (kamailio/kamailio#4304)

The case is a little bit different (it is made in response to this issue https://github.com/kamailio/kamailio/issues/4266):

The case:

When Kamailio receives a REGISTER request, it makes a parallel forking to registrars ( mid-registrar case, let's say ) The Kamailio catches the Auth response (401/407) from Registrars at the branch-failure route. If it authenticates and sends a REGISTER with an authentication header to the Registrar.

However, there are some cases where the first Registrar authenticates and sends a 200 OK response earlier than other Registrars send 401/407 responses.

existing logic doesn't give those responses being handled anyhow and discards them, as the transaction status already changed to 200, and logic hits this point: https://github.com/kamailio/kamailio/blob/master/src/modules/tm/t_reply.c#L1...

goto: discard;

Added code allows handling such responses within failure routes; however, it won't propagate failure replies to UAC, because the new code is >=300 https://github.com/kamailio/kamailio/blob/master/src/modules/tm/t_reply.c#L1... and the status of the transaction is already 200 https://github.com/kamailio/kamailio/blob/master/src/modules/tm/t_reply.c#L1... 200 responses to late parallel registrations also won't be propagated to UAC because the transaction status 200 already exists.

The description of the added code might be indeed messy - I'll try to improve the clarity of the comment

@ovoshlook pushed 1 commit.

8f1f5a901902b676f64924248e2dcaa7ae518df5 tm: Do not discard late negative replies for forked REGISTER

@ovoshlook pushed 1 commit.

6fd39a352fe1ee059389d0ca81fb67457eac68eb tm: Do not discard late forked REGISTER replies

@ovoshlook pushed 1 commit.

eb2db7710e82806d900bc7af2f2d10cf8ae33007 tm: Do not discard late forked REGISTER replies

henningw left a comment (kamailio/kamailio#4304)

I have re-run the checks. The check-commit is failing at an unrelated commit, and can be probably ignored. The check-format is failing at your commit it seems, some whitespace change maybe not correct? Just execute clang-format and force-push again.

@ovoshlook pushed 2 commits.

1a5dc96f772f884ae5f428ae77bb0755ae432fbe tm: Do not discard late forked REGISTER replies 83903a7f126fe05be1920361f3645509c751aa78 remove unneeded whitespace

ovoshlook left a comment (kamailio/kamailio#4304)

Thanks for the comment. That's a good point. Initially, I was thinking of covering all initial uncovered requests. However: - `t_should_relay_response` is quite a sensitive function for large changes. - The patch covers a particular edge case related to a particular issue. - Spreading the context may disrupt the clarity of the edge case and code. Moving forward with atomic features was a better approach for me.

According to the questions, it seems the feature's purpose is much clearer now. If no objections, then I'll cover the rest within this feature branch.