<p>Neels Hofmeyr has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/13591">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">fix: multiple initial CRCX<br><br>The first CRCX responds with the actual MGW endpoint name that is assigned (at<br>least for rtpbridge/*@mgw requests).<br><br>If multiple CRCX are scheduled at the same time on a fresh MGW endpoint, both<br>get fired with a '*' and each creates a separate MGW endpoint.<br><br>Make mgcp_client_endpoint_fsm avoid this, and schedule only one CRCX at first,<br>and the rest once the MGW endpoint name is fixated. It is thus possible to<br>safely issue two osmo_mgcpc_ep_ci_request() at the same time.<br><br>Change-Id: I92a9944acc96398acd6649f9c3c5badec5dd6dcc<br>---<br>M src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c<br>1 file changed, 86 insertions(+), 11 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/91/13591/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c b/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c</span><br><span>index a50491f..e8ca527 100644</span><br><span>--- a/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c</span><br><span>+++ b/src/libosmo-mgcp-client/mgcp_client_endpoint_fsm.c</span><br><span>@@ -101,6 +101,7 @@</span><br><span> struct osmo_fsm_inst *fi;</span><br><span> char endpoint[MGCP_ENDPOINT_MAXLEN];</span><br><span> const struct osmo_tdef *T_defs;</span><br><span style="color: hsl(120, 100%, 40%);">+ bool first_crcx_complete;</span><br><span> </span><br><span> struct osmo_mgcpc_ep_ci ci[USABLE_CI];</span><br><span> };</span><br><span>@@ -114,6 +115,9 @@</span><br><span> {}</span><br><span> };</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static void osmo_mgcpc_ep_count(struct osmo_mgcpc_ep *ep, int *occupied, int *pending_not_sent,</span><br><span style="color: hsl(120, 100%, 40%);">+ int *waiting_for_response);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static struct osmo_mgcpc_ep_ci *osmo_mgcpc_ep_check_ci(struct osmo_mgcpc_ep_ci *ci)</span><br><span> {</span><br><span> if (!ci)</span><br><span>@@ -323,6 +327,49 @@</span><br><span> osmo_fsm_inst_dispatch(notify, notify_failure, notify_data);</span><br><span> }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+static int update_endpoint_name(struct osmo_mgcpc_ep_ci *ci, const char *new_endpoint_name)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_mgcpc_ep *ep = ci->ep;</span><br><span style="color: hsl(120, 100%, 40%);">+ int rc;</span><br><span style="color: hsl(120, 100%, 40%);">+ int i;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!strcmp(ep->endpoint, new_endpoint_name)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Same endpoint name, nothing to do. */</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* The endpoint name should only change on the very first CRCX response. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ep->first_crcx_complete) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOG_CI(ci, LOGL_ERROR, "Reponse returned mismatching endpoint name."</span><br><span style="color: hsl(120, 100%, 40%);">+ " This is endpoint %s, instead received %s\n",</span><br><span style="color: hsl(120, 100%, 40%);">+ ep->endpoint, new_endpoint_name);</span><br><span style="color: hsl(120, 100%, 40%);">+ on_failure(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* This is the first CRCX response, update endpoint name. */</span><br><span style="color: hsl(120, 100%, 40%);">+ rc = OSMO_STRLCPY_ARRAY(ep->endpoint, new_endpoint_name);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (rc <= 0 || rc >= sizeof(ep->endpoint)) {</span><br><span style="color: hsl(120, 100%, 40%);">+ LOG_CI(ci, LOGL_ERROR, "Unable to copy endpoint name %s\n", osmo_quote_str(new_endpoint_name, -1));</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_mgcpc_ep_ci_dlcx(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ on_failure(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ return -ENOSPC;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make sure already pending requests use this updated endpoint name. */</span><br><span style="color: hsl(120, 100%, 40%);">+ for (i = 0; i < ARRAY_SIZE(ep->ci); i++) {</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_mgcpc_ep_ci *other_ci = &ep->ci[i];</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!other_ci->occupied)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!other_ci->pending)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (other_ci->sent)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_STRLCPY_ARRAY(other_ci->verb_info.endpoint, ep->endpoint);</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+ return 0;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> static void on_success(struct osmo_mgcpc_ep_ci *ci, void *data)</span><br><span> {</span><br><span> struct mgcp_conn_peer *rtp_info;</span><br><span>@@ -343,17 +390,12 @@</span><br><span> osmo_strlcpy(ci->mgcp_ci_str, mgcp_conn_get_ci(ci->mgcp_client_fi),</span><br><span> sizeof(ci->mgcp_ci_str));</span><br><span> if (rtp_info->endpoint[0]) {</span><br><span style="color: hsl(0, 100%, 40%);">- int rc;</span><br><span style="color: hsl(0, 100%, 40%);">- rc = osmo_strlcpy(ci->ep->endpoint, rtp_info->endpoint,</span><br><span style="color: hsl(0, 100%, 40%);">- sizeof(ci->ep->endpoint));</span><br><span style="color: hsl(0, 100%, 40%);">- if (rc <= 0 || rc >= sizeof(ci->ep->endpoint)) {</span><br><span style="color: hsl(0, 100%, 40%);">- LOG_CI(ci, LOGL_ERROR, "Unable to copy endpoint name '%s'\n",</span><br><span style="color: hsl(0, 100%, 40%);">- rtp_info->endpoint);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_mgcpc_ep_ci_dlcx(ci);</span><br><span style="color: hsl(0, 100%, 40%);">- on_failure(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* On errors, this instance might already be deallocated. Make sure to not access anything after</span><br><span style="color: hsl(120, 100%, 40%);">+ * error. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (update_endpoint_name(ci, rtp_info->endpoint))</span><br><span> return;</span><br><span style="color: hsl(0, 100%, 40%);">- }</span><br><span> }</span><br><span style="color: hsl(120, 100%, 40%);">+ ci->ep->first_crcx_complete = true;</span><br><span> break;</span><br><span> </span><br><span> default:</span><br><span>@@ -537,6 +579,7 @@</span><br><span> if (!ci->mgcp_client_fi){</span><br><span> LOG_CI_VERB(ci, LOGL_ERROR, "Cannot send\n");</span><br><span> on_failure(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span> }</span><br><span> osmo_fsm_inst_update_id(ci->mgcp_client_fi, ci->label);</span><br><span> break;</span><br><span>@@ -549,6 +592,7 @@</span><br><span> if (rc) {</span><br><span> LOG_CI_VERB(ci, LOGL_ERROR, "Cannot send (rc=%d %s)\n", rc, strerror(-rc));</span><br><span> on_failure(ci);</span><br><span style="color: hsl(120, 100%, 40%);">+ return -EINVAL;</span><br><span> }</span><br><span> break;</span><br><span> </span><br><span>@@ -641,16 +685,47 @@</span><br><span> </span><br><span> static void osmo_mgcpc_ep_fsm_wait_mgw_response_onenter(struct osmo_fsm_inst *fi, uint32_t prev_state)</span><br><span> {</span><br><span style="color: hsl(120, 100%, 40%);">+ static int re_enter = 0;</span><br><span style="color: hsl(120, 100%, 40%);">+ int rc;</span><br><span> int count = 0;</span><br><span> int i;</span><br><span> struct osmo_mgcpc_ep *ep = osmo_mgcpc_ep_fi_mgwep(fi);</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ re_enter++;</span><br><span style="color: hsl(120, 100%, 40%);">+ OSMO_ASSERT(re_enter < 10);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* The first CRCX gives us the endpoint name in the CRCX response. So we must wait for the first CRCX endpoint</span><br><span style="color: hsl(120, 100%, 40%);">+ * response to come in before sending any other MGCP requests -- otherwise we might end up creating new</span><br><span style="color: hsl(120, 100%, 40%);">+ * endpoints instead of acting on the same. This FSM always sends out N requests and waits for all of them to</span><br><span style="color: hsl(120, 100%, 40%);">+ * complete before sending out new requests. Hence we're safe when the very first time at most one request is</span><br><span style="color: hsl(120, 100%, 40%);">+ * sent (which needs to be a CRCX). */</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> for (i = 0; i < ARRAY_SIZE(ep->ci); i++) {</span><br><span style="color: hsl(0, 100%, 40%);">- count += send_verb(&ep->ci[i]);</span><br><span style="color: hsl(120, 100%, 40%);">+ struct osmo_mgcpc_ep_ci *ci = &ep->ci[i];</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make sure that only CRCX get dispatched if no CRCX were sent yet. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ep->first_crcx_complete) {</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ci->occupied && ci->verb != MGCP_VERB_CRCX)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ }</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+ rc = send_verb(&ep->ci[i]);</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Need to be careful not to access the instance after failure. Event chains may already have</span><br><span style="color: hsl(120, 100%, 40%);">+ * deallocated this memory. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (rc < 0)</span><br><span style="color: hsl(120, 100%, 40%);">+ return;</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!rc)</span><br><span style="color: hsl(120, 100%, 40%);">+ continue;</span><br><span style="color: hsl(120, 100%, 40%);">+ count++;</span><br><span style="color: hsl(120, 100%, 40%);">+ /* Make sure that we wait for the first CRCX response before dispatching more requests. */</span><br><span style="color: hsl(120, 100%, 40%);">+ if (!ep->first_crcx_complete)</span><br><span style="color: hsl(120, 100%, 40%);">+ break;</span><br><span> }</span><br><span> </span><br><span> LOG_MGCPC_EP(ep, LOGL_DEBUG, "Sent messages: %d\n", count);</span><br><span style="color: hsl(0, 100%, 40%);">- osmo_mgcpc_ep_fsm_check_state_chg_after_response(fi);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (ep->first_crcx_complete)</span><br><span style="color: hsl(120, 100%, 40%);">+ osmo_mgcpc_ep_fsm_check_state_chg_after_response(fi);</span><br><span style="color: hsl(120, 100%, 40%);">+ re_enter--;</span><br><span> }</span><br><span> </span><br><span> static void osmo_mgcpc_ep_fsm_handle_ci_events(struct osmo_fsm_inst *fi, uint32_t event, void *data)</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/13591">change 13591</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.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.osmocom.org/13591"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I92a9944acc96398acd6649f9c3c5badec5dd6dcc </div>
<div style="display:none"> Gerrit-Change-Number: 13591 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>