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