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

dialog: fix rtags of forking INVITE #1023

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

maximilianfridrich
Copy link
Contributor

@maximilianfridrich
Copy link
Contributor Author

A similar PR (#947) resulted in no ACKs being sent in some scenarios - see baresip/baresip#2772.

I tested this patch with the attached SIPP scenario (bob_100rel_hold.txt) and the BareSIP selftest (which checks for pending ACKs).

Still, ideally, this patch could be tested in a few different setups to ensure that ACKs are always sent correctly.

@cspiel1
Copy link
Collaborator

cspiel1 commented Dec 5, 2023

@maximilianfridrich please could you add a retest for re-INVITE which also checks that an ACK is sent/received?

@maximilianfridrich maximilianfridrich marked this pull request as draft December 10, 2023 11:12
@maximilianfridrich
Copy link
Contributor Author

Apparently there's still issues with ACKs to re-INVITEs, as stated by juha-h in baresip/baresip#2819. I have looked into that in the past and could not find the cause. I will try again.

@maximilianfridrich maximilianfridrich force-pushed the forking_100rel branch 2 times, most recently from 680444d to 5d8fc5d Compare December 11, 2023 13:03
@maximilianfridrich
Copy link
Contributor Author

I have finally found the ACK issue!

The issue was that the pl structs in dlg->route are initialized to point to a memory address within dlg->mb. In my patch I re-initialize dlg->mb and therefore the pointers in dlg->route were no longer valid. Now, after dlg->mb is reset in sip_dialog_update, dlg->route is also re-set to point into the new mbuf.

@maximilianfridrich maximilianfridrich marked this pull request as ready for review December 11, 2023 13:09
@cspiel1
Copy link
Collaborator

cspiel1 commented Dec 12, 2023

This test helps to detect missing ACKs: #1027

@sreimers
Copy link
Member

@maximilianfridrich can you rebase against main to check if ACK tests are passed.

@maximilianfridrich
Copy link
Contributor Author

@sreimers Done. Looks good.

@sreimers sreimers merged commit 1d23051 into baresip:main Dec 19, 2023
35 checks passed
@sreimers
Copy link
Member

Thanks!

@maximilianfridrich maximilianfridrich deleted the forking_100rel branch February 12, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad totag sent by baresip in the ACK and BYE
3 participants