<p>dexter has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25126">View Change</a></p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">mgcp_protocol: get rid of policy_cb and change_cb.<br><br>The two callback functions policy_cb and change_cb are essentially dead<br>code. They also make the code more difficult to read and understand.<br>Lets remove them.<br><br>Change-Id: I19f67db1c56473f47338b56114f6bbae8981d067<br>---<br>M include/osmocom/mgcp/mgcp.h<br>M include/osmocom/mgcp/mgcp_protocol.h<br>M src/libosmo-mgcp/mgcp_protocol.c<br>M tests/mgcp/mgcp_test.c<br>4 files changed, 17 insertions(+), 110 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/26/25126/1</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/include/osmocom/mgcp/mgcp.h b/include/osmocom/mgcp/mgcp.h</span><br><span>index 29963cc..b3f2eb5 100644</span><br><span>--- a/include/osmocom/mgcp/mgcp.h</span><br><span>+++ b/include/osmocom/mgcp/mgcp.h</span><br><span>@@ -62,8 +62,6 @@</span><br><span> #define MGCP_POLICY_REJECT      5</span><br><span> #define MGCP_POLICY_DEFER  6</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-typedef int (*mgcp_change)(struct mgcp_endpoint *endp, int state);</span><br><span style="color: hsl(0, 100%, 40%);">-typedef int (*mgcp_policy)(struct mgcp_endpoint *endp, int state, const char *transaction_id);</span><br><span> typedef int (*mgcp_reset)(struct mgcp_trunk *cfg);</span><br><span> typedef int (*mgcp_rqnt)(struct mgcp_endpoint *endp, char tone);</span><br><span> </span><br><span>@@ -147,8 +145,6 @@</span><br><span> </span><br><span>        int force_ptime;</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-    mgcp_change change_cb;</span><br><span style="color: hsl(0, 100%, 40%);">-  mgcp_policy policy_cb;</span><br><span>       mgcp_reset reset_cb;</span><br><span>         mgcp_rqnt rqnt_cb;</span><br><span>   void *data;</span><br><span>diff --git a/include/osmocom/mgcp/mgcp_protocol.h b/include/osmocom/mgcp/mgcp_protocol.h</span><br><span>index 7ab283d..df134ab 100644</span><br><span>--- a/include/osmocom/mgcp/mgcp_protocol.h</span><br><span>+++ b/include/osmocom/mgcp/mgcp_protocol.h</span><br><span>@@ -16,6 +16,8 @@</span><br><span>         int pkt_period_max; /* time in ms */</span><br><span> };</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+char *mgcp_debug_get_last_endpoint_name(void);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> char *get_lco_identifier(const char *options);</span><br><span> int check_local_cx_options(void *ctx, const char *options);</span><br><span> </span><br><span>diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>index e69c00f..1343b61 100644</span><br><span>--- a/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>+++ b/src/libosmo-mgcp/mgcp_protocol.c</span><br><span>@@ -46,6 +46,16 @@</span><br><span> #include <osmocom/mgcp/mgcp_codec.h></span><br><span> #include <osmocom/mgcp/mgcp_conn.h></span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+/* Contains the last successfuly resolved endpoint name. This variable is used</span><br><span style="color: hsl(120, 100%, 40%);">+ * for the unit-tests to verify that the endpoint was correctly resolved. */</span><br><span style="color: hsl(120, 100%, 40%);">+static char debug_last_endpoint_name[MGCP_ENDPOINT_MAXLEN];</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+/* Called from unit-tests only */</span><br><span style="color: hsl(120, 100%, 40%);">+char *mgcp_debug_get_last_endpoint_name(void)</span><br><span style="color: hsl(120, 100%, 40%);">+{</span><br><span style="color: hsl(120, 100%, 40%);">+      return debug_last_endpoint_name;</span><br><span style="color: hsl(120, 100%, 40%);">+}</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span> /* A combination of LOGPENDP and LOGPTRUNK that automatically falls back to</span><br><span>  * LOGPTRUNK when the endp parameter is NULL */</span><br><span> #define LOGPEPTR(endp, trunk, cat, level, fmt, args...) \</span><br><span>@@ -328,6 +338,8 @@</span><br><span>   struct msgb *resp = NULL;</span><br><span>    char *data;</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+       debug_last_endpoint_name[0] = '\0';</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span>        /* Count all messages, even incorect ones */</span><br><span>         rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_GENERAL_RX_MSGS_TOTAL));</span><br><span> </span><br><span>@@ -395,6 +407,7 @@</span><br><span>                       return create_err_response(NULL, -rq.mgcp_cause, rq.name, pdata.trans);</span><br><span>              }</span><br><span>    } else {</span><br><span style="color: hsl(120, 100%, 40%);">+              osmo_strlcpy(debug_last_endpoint_name, rq.endp->name, sizeof(debug_last_endpoint_name));</span><br><span>          rq.trunk = rq.endp->trunk;</span><br><span>                rq.mgcp_cause = 0;</span><br><span> </span><br><span>@@ -1050,32 +1063,8 @@</span><br><span>              goto error2;</span><br><span>         }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   /* policy CB */</span><br><span style="color: hsl(0, 100%, 40%);">- if (pdata->cfg->policy_cb) {</span><br><span style="color: hsl(0, 100%, 40%);">-              int rc;</span><br><span style="color: hsl(0, 100%, 40%);">-         rc = pdata->cfg->policy_cb(endp, MGCP_ENDP_CRCX, pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                switch (rc) {</span><br><span style="color: hsl(0, 100%, 40%);">-           case MGCP_POLICY_REJECT:</span><br><span style="color: hsl(0, 100%, 40%);">-                        LOGPCONN(_conn, DLMGCP, LOGL_NOTICE,</span><br><span style="color: hsl(0, 100%, 40%);">-                             "CRCX: CRCX rejected by policy\n");</span><br><span style="color: hsl(0, 100%, 40%);">-                  mgcp_endp_release(endp);</span><br><span style="color: hsl(0, 100%, 40%);">-                        rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_CRCX_FAIL_REJECTED_BY_POLICY));</span><br><span style="color: hsl(0, 100%, 40%);">-                     return create_err_response(endp, 400, "CRCX", pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                       break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_DEFER:</span><br><span style="color: hsl(0, 100%, 40%);">-                 /* stop processing */</span><br><span style="color: hsl(0, 100%, 40%);">-                   return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-                    break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_CONT:</span><br><span style="color: hsl(0, 100%, 40%);">-                  /* just continue */</span><br><span style="color: hsl(0, 100%, 40%);">-                     break;</span><br><span style="color: hsl(0, 100%, 40%);">-          }</span><br><span style="color: hsl(0, 100%, 40%);">-       }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>    LOGPCONN(conn->conn, DLMGCP, LOGL_DEBUG,</span><br><span>           "CRCX: Creating connection: port: %u\n", conn->end.local_port);</span><br><span style="color: hsl(0, 100%, 40%);">-   if (pdata->cfg->change_cb)</span><br><span style="color: hsl(0, 100%, 40%);">-                pdata->cfg->change_cb(endp, MGCP_ENDP_CRCX);</span><br><span> </span><br><span>       /* Send dummy packet, see also comments in mgcp_keepalive_timer_cb() */</span><br><span>      OSMO_ASSERT(trunk->keepalive_interval >= MGCP_KEEPALIVE_ONCE);</span><br><span>@@ -1290,40 +1279,11 @@</span><br><span>               goto error3;</span><br><span>         }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-   /* policy CB */</span><br><span style="color: hsl(0, 100%, 40%);">- if (pdata->cfg->policy_cb) {</span><br><span style="color: hsl(0, 100%, 40%);">-              int rc;</span><br><span style="color: hsl(0, 100%, 40%);">-         rc = pdata->cfg->policy_cb(endp, MGCP_ENDP_MDCX, pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                switch (rc) {</span><br><span style="color: hsl(0, 100%, 40%);">-           case MGCP_POLICY_REJECT:</span><br><span style="color: hsl(0, 100%, 40%);">-                        LOGPCONN(conn->conn, DLMGCP, LOGL_NOTICE,</span><br><span style="color: hsl(0, 100%, 40%);">-                             "MDCX: rejected by policy\n");</span><br><span style="color: hsl(0, 100%, 40%);">-                       rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_MDCX_FAIL_REJECTED_BY_POLICY));</span><br><span style="color: hsl(0, 100%, 40%);">-                     if (silent)</span><br><span style="color: hsl(0, 100%, 40%);">-                             goto out_silent;</span><br><span style="color: hsl(0, 100%, 40%);">-                        return create_err_response(endp, 400, "MDCX", pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                       break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_DEFER:</span><br><span style="color: hsl(0, 100%, 40%);">-                 /* stop processing */</span><br><span style="color: hsl(0, 100%, 40%);">-                   LOGPCONN(conn->conn, DLMGCP, LOGL_DEBUG,</span><br><span style="color: hsl(0, 100%, 40%);">-                              "MDCX: deferred by policy\n");</span><br><span style="color: hsl(0, 100%, 40%);">-                       rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_MDCX_DEFERRED_BY_POLICY));</span><br><span style="color: hsl(0, 100%, 40%);">-                  return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-                    break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_CONT:</span><br><span style="color: hsl(0, 100%, 40%);">-                  /* just continue */</span><br><span style="color: hsl(0, 100%, 40%);">-                     break;</span><br><span style="color: hsl(0, 100%, 40%);">-          }</span><br><span style="color: hsl(0, 100%, 40%);">-       }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>    mgcp_rtp_end_config(endp, 1, &conn->end);</span><br><span> </span><br><span>         /* modify */</span><br><span>         LOGPCONN(conn->conn, DLMGCP, LOGL_DEBUG,</span><br><span>           "MDCX: modified conn:%s\n", mgcp_conn_dump(conn->conn));</span><br><span style="color: hsl(0, 100%, 40%);">-  if (pdata->cfg->change_cb)</span><br><span style="color: hsl(0, 100%, 40%);">-                pdata->cfg->change_cb(endp, MGCP_ENDP_MDCX);</span><br><span> </span><br><span>       /* Send dummy packet, see also comments in mgcp_keepalive_timer_cb() */</span><br><span>      OSMO_ASSERT(trunk->keepalive_interval >= MGCP_KEEPALIVE_ONCE);</span><br><span>@@ -1431,29 +1391,6 @@</span><br><span>                }</span><br><span>    }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   /* policy CB */</span><br><span style="color: hsl(0, 100%, 40%);">- if (pdata->cfg->policy_cb) {</span><br><span style="color: hsl(0, 100%, 40%);">-              int rc;</span><br><span style="color: hsl(0, 100%, 40%);">-         rc = pdata->cfg->policy_cb(endp, MGCP_ENDP_DLCX, pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                switch (rc) {</span><br><span style="color: hsl(0, 100%, 40%);">-           case MGCP_POLICY_REJECT:</span><br><span style="color: hsl(0, 100%, 40%);">-                        LOGPENDP(endp, DLMGCP, LOGL_NOTICE, "DLCX: rejected by policy\n");</span><br><span style="color: hsl(0, 100%, 40%);">-                    rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_DLCX_FAIL_REJECTED_BY_POLICY));</span><br><span style="color: hsl(0, 100%, 40%);">-                     if (silent)</span><br><span style="color: hsl(0, 100%, 40%);">-                             goto out_silent;</span><br><span style="color: hsl(0, 100%, 40%);">-                        return create_err_response(endp, 400, "DLCX", pdata->trans);</span><br><span style="color: hsl(0, 100%, 40%);">-                       break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_DEFER:</span><br><span style="color: hsl(0, 100%, 40%);">-                 /* stop processing */</span><br><span style="color: hsl(0, 100%, 40%);">-                   rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_DLCX_DEFERRED_BY_POLICY));</span><br><span style="color: hsl(0, 100%, 40%);">-                  return NULL;</span><br><span style="color: hsl(0, 100%, 40%);">-                    break;</span><br><span style="color: hsl(0, 100%, 40%);">-          case MGCP_POLICY_CONT:</span><br><span style="color: hsl(0, 100%, 40%);">-                  /* just continue */</span><br><span style="color: hsl(0, 100%, 40%);">-                     break;</span><br><span style="color: hsl(0, 100%, 40%);">-          }</span><br><span style="color: hsl(0, 100%, 40%);">-       }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>    /* Handle wildcarded DLCX that refers to the whole trunk. This means</span><br><span>          * that we walk over all endpoints on the trunk in order to drop all</span><br><span>          * connections on the trunk. (see also RFC3435 Annex F.7) */</span><br><span>@@ -1515,9 +1452,6 @@</span><br><span>                 LOGPENDP(endp, DLMGCP, LOGL_DEBUG, "DLCX: endpoint released\n");</span><br><span>   }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   if (pdata->cfg->change_cb)</span><br><span style="color: hsl(0, 100%, 40%);">-                pdata->cfg->change_cb(endp, MGCP_ENDP_DLCX);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>   rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, MGCP_DLCX_SUCCESS));</span><br><span>  if (silent)</span><br><span>          goto out_silent;</span><br><span>diff --git a/tests/mgcp/mgcp_test.c b/tests/mgcp/mgcp_test.c</span><br><span>index 26fcc2a..1c0d3cc 100644</span><br><span>--- a/tests/mgcp/mgcp_test.c</span><br><span>+++ b/tests/mgcp/mgcp_test.c</span><br><span>@@ -614,28 +614,6 @@</span><br><span>         return msg;</span><br><span> }</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-static char last_endpoint[MGCP_ENDPOINT_MAXLEN];</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-static int mgcp_test_policy_cb(struct mgcp_endpoint *endp,</span><br><span style="color: hsl(0, 100%, 40%);">-                         int state, const char *transaction_id)</span><br><span style="color: hsl(0, 100%, 40%);">-{</span><br><span style="color: hsl(0, 100%, 40%);">-  unsigned int i;</span><br><span style="color: hsl(0, 100%, 40%);">- struct mgcp_trunk *trunk;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-       fprintf(stderr, "Policy CB got state %d on endpoint %s\n",</span><br><span style="color: hsl(0, 100%, 40%);">-            state, endp->name);</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-  trunk = endp->trunk;</span><br><span style="color: hsl(0, 100%, 40%);">- last_endpoint[0] = '\0';</span><br><span style="color: hsl(0, 100%, 40%);">-        for (i = 0; i < trunk->number_endpoints; i++) {</span><br><span style="color: hsl(0, 100%, 40%);">-           if (strcmp(endp->name, trunk->endpoints[i]->name) == 0)</span><br><span style="color: hsl(0, 100%, 40%);">-                        osmo_strlcpy(last_endpoint, trunk->endpoints[i]->name,</span><br><span style="color: hsl(0, 100%, 40%);">-                                 sizeof(last_endpoint));</span><br><span style="color: hsl(0, 100%, 40%);">-    }</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span style="color: hsl(0, 100%, 40%);">-       return MGCP_POLICY_CONT;</span><br><span style="color: hsl(0, 100%, 40%);">-}</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span> static int dummy_packets = 0;</span><br><span> /* override and forward */</span><br><span> ssize_t sendto(int sockfd, const void *buf, size_t len, int flags,</span><br><span>@@ -792,13 +770,13 @@</span><br><span>       struct mgcp_conn_rtp *conn = NULL;</span><br><span>   char last_conn_id[256];</span><br><span>      int rc;</span><br><span style="color: hsl(120, 100%, 40%);">+       char *last_endpoint = mgcp_debug_get_last_endpoint_name();</span><br><span> </span><br><span>       cfg = mgcp_config_alloc();</span><br><span>   trunk = mgcp_trunk_by_num(cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID);</span><br><span> </span><br><span>  trunk->v.vty_number_endpoints = 64;</span><br><span>         mgcp_trunk_equip(trunk);</span><br><span style="color: hsl(0, 100%, 40%);">-      cfg->policy_cb = mgcp_test_policy_cb;</span><br><span> </span><br><span>         memset(last_conn_id, 0, sizeof(last_conn_id));</span><br><span> </span><br><span>@@ -810,7 +788,6 @@</span><br><span>             printf("\n================================================\n");</span><br><span>            printf("Testing %s\n", t->name);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-               last_endpoint[0] = '\0';</span><br><span>             dummy_packets = 0;</span><br><span> </span><br><span>               osmo_talloc_replace_string(cfg, &trunk->audio_fmtp_extra,</span><br><span>@@ -1411,6 +1388,7 @@</span><br><span>     struct in_addr addr;</span><br><span>         struct mgcp_conn_rtp *conn = NULL;</span><br><span>   char conn_id[256];</span><br><span style="color: hsl(120, 100%, 40%);">+    char *last_endpoint = mgcp_debug_get_last_endpoint_name();</span><br><span> </span><br><span>       printf("Testing multiple payload types\n");</span><br><span> </span><br><span>@@ -1418,7 +1396,6 @@</span><br><span>    trunk = mgcp_trunk_by_num(cfg, MGCP_TRUNK_VIRTUAL, MGCP_VIRT_TRUNK_ID);</span><br><span>      trunk->v.vty_number_endpoints = 64;</span><br><span>         mgcp_trunk_equip(trunk);</span><br><span style="color: hsl(0, 100%, 40%);">-      cfg->policy_cb = mgcp_test_policy_cb;</span><br><span> </span><br><span>         /* Allocate endpoint 1@mgw with two codecs */</span><br><span>        last_endpoint[0] = '\0';</span><br><span>@@ -1620,8 +1597,6 @@</span><br><span>     trunk->audio_send_name = 0;</span><br><span>         mgcp_trunk_equip(trunk);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-  cfg->policy_cb = mgcp_test_policy_cb;</span><br><span style="color: hsl(0, 100%, 40%);">-</span><br><span>     inp = create_msg(CRCX, NULL);</span><br><span>        msg = mgcp_handle_message(cfg, inp);</span><br><span> </span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25126">change 25126</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/c/osmo-mgw/+/25126"/><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-Change-Id: I19f67db1c56473f47338b56114f6bbae8981d067 </div>
<div style="display:none"> Gerrit-Change-Number: 25126 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>