Skip to content

refactor(pjsua): commit the new Contact in update_regc_contact() - #26

Open
laconicman wants to merge 2 commits into
masterfrom
refactor/regc-contact-rollback-r2
Open

laconicman wants to merge 2 commits into
masterfrom
refactor/regc-contact-rollback-r2

Conversation

@laconicman

@laconicman laconicman commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

Follow-up to pjsip#5281, as agreed in its review.

  • update_regc_contact() takes the candidate Contact and commits it to acc->contact only after the regc accepts it, so the rollback is in one place; acc_check_nat_addr() and auto_rereg_timer_cb() no longer save and restore acc->contact (review).
  • So acc_check_nat_addr() no longer captures prev_contact after destroy_regc() has cleared it in UNREGISTER mode (review).
  • The dead else that aliased reg_contact to contact is removed (len always exceeds acc->contact.slen), together with the OUTBOUND_WANTED store it left dead. Its body is dedented, so the diff reads best with whitespace hidden.

No behaviour change intended.

Verified on macOS arm64 (Apple clang 21), each build against the previous one (unpatched master, then the first review round):

Check Result
make -j3 no new warnings
pjsip-test pjsua_acc_test pass
SIPp uas-register-outbound-norequire (tests/pjsua) pass
Scripted TCP registrar: 200 without Require: outbound, ACTIVE -> NA, 439 failover, initial 439, NAT rewrite, NAT rewrite after 401 REGISTERs identical

Not run here: Linux and Windows builds, and the full test suites.


Devin Review

Pass the candidate Contact into update_regc_contact() and commit it to
acc->contact only after the regc has accepted it, so the rollback lives
in one place instead of in acc_check_nat_addr() and
auto_rereg_timer_cb(). This also drops acc_check_nat_addr()'s
prev_contact, which in UNREGISTER mode was captured after
destroy_regc() had already cleared acc->contact.

Remove the else branch that aliased reg_contact to contact: len always
exceeds acc->contact.slen, so it never ran.

No behaviour change.
- Say that only a push can be refused, and that without push the
  Contact is committed before the regc has seen it.
- Log "Contact for acc %d updated" only once the push has succeeded.
- Treat &acc->contact as new_contact like NULL: pj_strdup_with_null()
  cannot copy a string onto itself.
- Drop the OUTBOUND_WANTED store that the removed else left dead, and
  the bare block around its body.
- Rename prev_contact to prev_reg_contact.
- Note why auto_rereg_timer_cb() carries on after a refusal.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

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.

1 participant