-
Notifications
You must be signed in to change notification settings - Fork 1.2k
res_pjsip: Redirect module and handling for MESSAGEs #1580
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
base: master
Are you sure you want to change the base?
res_pjsip: Redirect module and handling for MESSAGEs #1580
Conversation
|
cherry-pick-to: 20 |
|
Workflow PRCheck completed successfully |
683e974 to
788d3d7
Compare
|
Workflow PRCheck completed successfully |
788d3d7 to
f07f2e0
Compare
|
Workflow PRCheck completed successfully |
| /*! | ||
| * \brief Maximum size for a URI string | ||
| */ | ||
| #define AST_SIP_MAX_URI_SIZE 512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason not to use the PJSIP defined one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there was a PJSIP defined one but I couldn't find it. I found it now and will use it. Thanks.
| * SIP 3xx redirects, including visited URIs for loop detection, | ||
| * pending contacts for retry logic, and hop counting. | ||
| */ | ||
| struct ast_sip_redirect_context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context has a rather specific meaning within Asterisk, and I must admit on reading this name I thought it meant dialplan context and was confused until the brief description sunk in. I think a different name would eliminate that confusion. "ast_sip_redirect_state"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, thanks and sorry about the confusion. I'll rename everything to state.
| * \brief Check if redirect should be followed based on endpoint configuration | ||
| * | ||
| * \param endpoint The SIP endpoint | ||
| * \param status_code The response status code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The response status code from what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarified the doc comment: "The status code from a SIP response message (e.g. 305)"
res/res_pjsip/redirect.c
Outdated
| * \param contacts List to populate with parsed contacts | ||
| * \return Number of valid contacts found | ||
| */ | ||
| static int parse_redirect_contacts(pjsip_rx_data *rdata, struct redirect_contact_list *contacts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass in the state/context pointer, and use the endpoint to add name to the logging
I'm kind of a broken record lately with logging, but giving information on which endpoint is involved can be very valuable if someone goes back to try to understand what happened on a well used system
The same applies for the code going forward after this comment, I'm not going to comment on each one but I urge you to go back to each and try to make them more specific/detailed including endpoint name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I tried to add the endpoint ID to the logs (mostly within the prose). Let me know if this is okay or if you would like it differently (e.g. a consistent format).
res/res_pjsip/redirect.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| ast_log(LOG_NOTICE, "Redirect: found %d Contact header%s\n", contact_count, contact_count == 1 ? "" : "s"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Done.
res/res_pjsip_messaging.c
Outdated
| { | ||
| struct message_response_data *resp_data; | ||
|
|
||
| resp_data = ao2_alloc(sizeof(*resp_data), message_response_data_destroy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be allocated without a lock using ao2_alloc_options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done.
res/res_pjsip_messaging.c
Outdated
| struct msg_data *mdata = data; /* The caller holds a reference */ | ||
| /* Callback data for redirect handling */ | ||
| struct message_response_data *resp_data; | ||
| char content_type_buf[128]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did 128 come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arbitrary. I thought that should be enough for the Content-Type header which contains a MIME type (potentially with parameters). I changed it now to a malloced ast_str which can grow if needed.
f07f2e0 to
cd6d080
Compare
|
Thanks for the review @jcolp! I've incorporated your feedback and responded to your comments. Let me know what you think. |
|
Workflow PRCheck completed successfully |
res/res_pjsip/pjsip_config.xml
Outdated
| and also supporting multiple potential redirect targets. The con is that since redirection occurs | ||
| within chan_pjsip redirecting information is not forwarded and redirection can not be | ||
| prevented. | ||
| prevented. Further, if <literal>uri_pjsip</literal> is configured, 3xx responses to SIP MESSAGEs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the follow_redirect_methods is good.
res/res_pjsip_messaging.c
Outdated
|
|
||
| /* Check event type */ | ||
| if (e->body.tsx_state.type == PJSIP_EVENT_TIMER) { | ||
| ast_debug(1, "MESSAGE request timed out\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to include endpoint if present and be more specific "MESSAGE request timed out on sending to endpoint %s\n"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This commit introduces a new redirect handling module that provides infrastructure for following SIP 3xx redirect responses. The redirect functionality respects the endpoint's redirect_method setting and only follows redirects when set to 'uri_pjsip'. This infrastructure can be used by any PJSIP module that needs to handle 3xx redirect responses.
cd6d080 to
c39a0bb
Compare
|
Thanks again for another review @jcolp! I've incorporated your feedback, created the new config option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attention! This pull request may contain issues that could prevent it from being accepted. Please review the checklist below and take the recommended action. If you believe any of these are not applicable, just add a comment and let us know.
- An Alembic change was detected but a commit message UpgradeNote with at least one of the 'alembic', 'database' or 'schema' keywords wasn't found. Please add an UpgradeNote to the commit message that mentions one of those keywords notifying users that there's a database schema change.
Documentation:
|
Workflow PRCheck completed successfully |
This commit integrates the redirect module into res_pjsip_messaging to enable following 3xx redirect responses for outgoing SIP MESSAGEs. When follow_redirect_methods contains 'message' on an endpoint, Asterisk will now follow 3xx redirect responses for MESSAGEs, similar to how it behaves for INVITE responses. Resolves: asterisk#1576 UserNote: A new pjsip endpoint option follow_redirect_methods was added. This option is a comma-delimited, case-insensitive list of SIP methods for which SIP 3XX redirect responses are followed. An alembic upgrade script has been added for adding this new option to the Asterisk database.
c39a0bb to
dfbeed0
Compare
|
Workflow PRCheck failed |
res_pjsip: Introduce redirect module for handling 3xx responses
This commit introduces a new redirect handling module that provides
infrastructure for following SIP 3xx redirect responses. The redirect
functionality respects the endpoint's redirect_method setting and only
follows redirects when set to 'uri_pjsip'. This infrastructure can be
used by any PJSIP module that needs to handle 3xx redirect responses.
res_pjsip_messaging: Add support for following 3xx redirects
This commit integrates the redirect module into res_pjsip_messaging
to enable following 3xx redirect responses for outgoing SIP MESSAGEs.
When follow_redirect_methods contains 'message' on an endpoint, Asterisk
will now follow 3xx redirect responses for MESSAGEs, similar to how
it behaves for INVITE responses.
Resolves: #1576
UserNote: A new pjsip endpoint option follow_redirect_methods was added.
This option is a comma-delimited, case-insensitive list of SIP methods
for which SIP 3XX redirect responses are followed. An alembic upgrade
script has been added for adding this new option to the Asterisk
database.