<p> Attention is currently required from: N A, Maximilian Fridrich. </p>
<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/18838">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18838/comment/b7f5c60b_fb6082e9">Patch Set #5, Line 422:</a> <code style="font-family:monospace,monospace">AST_SIP_100REL_SUPPORTED</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Might want to be a bit clearer that AST_SIP_100REL_SUPPORTED is for uac.<br>You should also note what the config value is for each enum. I.E. "yes", "no", "required", "peer_supported".</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip/pjsip_config.xml:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18838/comment/6ce99959_4f23507f">Patch Set #5, Line 35:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> <enum name="no"><br> <para>If set to <literal>no</literal>, do not support transmission of<br> reliable provisional responses.</para><br> </enum><br> <enum name="required"><br> <para>If set to <literal>required</literal>, require provisional responses to<br> be sent reliably.</para><br> </enum><br> <enum name="peer_supported"><br> <para>If set to <literal>peer_supported</literal>, send provisional responses<br> reliably if the request by the peer contained 100rel in the Supported or<br> Require header. If the request by the peer did not contain 100rel in the Supported<br> and Require header, send responses them normally. Requests originating from Asterisk<br> will contain 100rel in the Supported header.</para><br> </enum><br> <enum name="yes"><br> <para>If set to <literal>yes</literal>, indicate the support of reliable provisional<br> responses and PRACK them if required by the peer. If the peer mereley indicates<br> support of 100rel in the Supported header, do not send 1xx responses reliable, but<br> indicate the support in the Supported header.</para><br> </enum><br> </enumlist><br> </description><br><br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">For each option, you should explicitly state the behavior when Asterisk is the UAC and when it's the UAS.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18838/comment/fd6a0d5b_2e577a3f">Patch Set #5, Line 201:</a> <code style="font-family:monospace,monospace">static int prack_to_str(const void *obj, const intptr_t *args, char **buf)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to update this function as well to provide the reverse mapping to string.<br>You should convert this to use the new 100rel parameter as the source at the same time.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_session.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18838/comment/657b6acf_b6701084">Patch Set #5, Line 3804:</a> <code style="font-family:monospace,monospace"> if (endpoint->rel100 == AST_SIP_100REL_PEER_SUPPORTED && rdata->msg_info.supported != NULL) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think a comment explaining what's going on would be appropriate.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18838">change 18838</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/18838"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Id6d95ffa8f00dab118e0b386146e99f254f287ad </div>
<div style="display:none"> Gerrit-Change-Number: 18838 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Maximilian Fridrich <m.fridrich@commend.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Maximilian Fridrich <m.fridrich@commend.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 07 Sep 2022 12:37:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>