Repository navigation
refactor(pjsua): commit the new Contact in update_regc_contact() - #5311
Conversation
| * With push, also load it into the regc; pass PJ_FALSE where the regc takes | ||
| * it from pjsip_regc_init() or regc_tsx_cb()'s param->contact instead. | ||
| * Returns PJ_FALSE if the regc refused it, leaving reg_contact as it was. | ||
| * Returns PJ_FALSE if the regc refused it, leaving the account as it was; |
There was a problem hiding this comment.
"leaving the account as it was" only holds with push=PJ_TRUE. With push=PJ_FALSE the function always returns PJ_TRUE and commits acc->contact/acc->reg_contact before the regc has seen them, e.g. in pjsua_regc_init() where pjsip_regc_init() may still reject reg_contact afterwards, or in the ALWAYS_UPDATE path where regc_tsx_cb hands param->contact over later. Should the comment say the refusal guarantee only applies to push?
There was a problem hiding this comment.
Yes: only a push can be refused. The comment now says so, and that without push the Contact is committed before the regc has seen it.
| @@ -2294,14 +2299,6 @@ static pj_bool_t update_regc_contact(pjsua_acc *acc, pj_bool_t push) | |||
| acc->index, | |||
| (int)acc->reg_contact.slen, | |||
| acc->reg_contact.ptr)); | |||
There was a problem hiding this comment.
"Contact for acc %d updated" is logged before the regc push below, which can still refuse and roll back. On a refusal the log shows the new Contact followed by "Failed updating registration Contact", while the account keeps the old one. Maybe move the log after the push succeeds?
There was a problem hiding this comment.
Moved after the push, so a refusal now logs only "Failed updating registration Contact".
| pj_strdup_with_null(acc->pool, &acc->contact, &tmp_contact); | ||
| if (!update_regc_contact(acc, PJ_TRUE)) | ||
| acc->contact = prev_contact; | ||
| update_regc_contact(acc, &tmp_contact, PJ_TRUE); |
There was a problem hiding this comment.
The return value is ignored here, so if the regc refuses the new Contact we go on to re-register with the old one and don't retry. Same at update_rfc5626_status() (line 3023). The old code behaved the same way, so this isn't a regression. Now that the helper reports refusal explicitly, is ignoring it intended here, or should this site e.g. schedule_reregistration() like the reinit failure branch above?
There was a problem hiding this comment.
Intended; there is now a one-line comment at the call. set_contact() refuses only a Contact that does not parse (PJSIP_EINVALIDURI), so the result depends on the input alone: rescheduling would just repeat the refusal and never send a REGISTER, while carrying on re-registers with the Contact the regc kept (it validates before touching its list), under the same retry rules as any other REGISTER. In update_rfc5626_status() the candidate is the already accepted Contact minus reg-id and +sip.instance, so a refusal is not expected there.
| } | ||
| } | ||
| if (new_contact) | ||
| pj_strdup_with_null(acc->pool, &acc->contact, new_contact); |
There was a problem hiding this comment.
Minor: if a future caller passes &acc->contact as new_contact (the doc's "NULL: acc->contact" sort of invites it), pj_strdup_with_null() copies onto itself: dst->ptr is set to a fresh buffer before copying from src->ptr, which is now that same buffer, so the Contact becomes uninitialised bytes. No current caller does this. Maybe pj_assert(new_contact != &acc->contact), or treat it like NULL?
There was a problem hiding this comment.
Treated like NULL now. An assert would compile out in release builds and still copy garbage.
| @@ -2191,20 +2196,20 @@ static pj_bool_t update_regc_contact(pjsua_acc *acc, pj_bool_t push) | |||
| pj_str_t reg_contact; | |||
|
|
|||
| acc->rfc5626_status = OUTBOUND_WANTED; | |||
There was a problem hiding this comment.
Minor: with the else branch gone, this store is dead. The status is always overwritten below (WANTED at 2274 or NA at 2282). Could drop it, and the bare { } block it leaves, so the status is set once from need_outbound.
There was a problem hiding this comment.
Dropped, together with the bare block. Its body is dedented, so the diff reads best with whitespace hidden.
| pjsua_acc_config *acc_cfg = &acc->cfg; | ||
| const pj_str_t *contact = new_contact ? new_contact : &acc->contact; | ||
| pj_bool_t need_outbound = PJ_FALSE; | ||
| pj_str_t prev_contact = acc->reg_contact; |
There was a problem hiding this comment.
Nit: prev_contact holds the previous acc->reg_contact, not acc->contact. With contact and new_contact also in scope now, prev_reg_contact would make it clearer which one the refusal path restores.
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. The OUTBOUND_WANTED store it left dead goes too. Log the new Contact only once the regc has accepted it, and treat &acc->contact as new_contact like NULL. No change on the wire.
e18e575 to
1fe52ea
Compare
3 participants
Follow-up to #5281, as agreed in its review.
update_regc_contact()takes the candidate Contact and commits it toacc->contactonly after the regc accepts it, so the rollback is in one place;acc_check_nat_addr()andauto_rereg_timer_cb()no longer save and restoreacc->contact(review).acc_check_nat_addr()no longer capturesprev_contactafterdestroy_regc()has cleared it in UNREGISTER mode (review).elsethat aliasedreg_contacttocontactis removed (lenalways exceedsacc->contact.slen), together with theOUTBOUND_WANTEDstore 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):
make -j3pjsip-test pjsua_acc_testuas-register-outbound-norequire(tests/pjsua)Require: outbound, ACTIVE -> NA, 439 failover, initial 439, NAT rewrite, NAT rewrite after 401Not run here: Linux and Windows builds, and the full test suites.