Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/4617 to look at the new patch set (#3). cosmetic: make dummy packet handling more explicit The way how osmo-mgw decides when to send a dummy packet and when not is not very obvious. use more explicit if statements, and define constants. Also add comments that explain how it works. Change-Id: Ie7ee9409baec50a09fb357d655b5253434fae924 --- M include/osmocom/mgcp/mgcp.h M src/libosmo-mgcp/mgcp_protocol.c M src/libosmo-mgcp/mgcp_vty.c 3 files changed, 37 insertions(+), 8 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/17/4617/3 diff --git a/include/osmocom/mgcp/mgcp.h b/include/osmocom/mgcp/mgcp.h index 83505a2..59147f0 100644 --- a/include/osmocom/mgcp/mgcp.h +++ b/include/osmocom/mgcp/mgcp.h @@ -98,7 +98,17 @@ int last_port; }; +/* There are up to three modes in which the keep-alive dummy packet can be + * sent. The beviour is controlled viw the keepalive_interval member of the + * trunk config. If that member is set to 0 (MGCP_KEEPALIVE_NEVER) no dummy- + * packet is sent at all and the timer that sends regular dummy packets + * is no longer scheduled. If the keepalive_interval is set to -1, only + * one dummy packet is sent when an CRCX or an MDCX is performed. No timer + * is scheduled. For all vales greater 0, the a timer is scheduled and the + * value is used as interval. See also mgcp_keepalive_timer_cb(), + * handle_modify_con(), and handle_create_con() */ #define MGCP_KEEPALIVE_ONCE (-1) +#define MGCP_KEEPALIVE_NEVER 0 struct mgcp_trunk_config { struct llist_head entry; diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c index 9c92c65..4826790 100644 --- a/src/libosmo-mgcp/mgcp_protocol.c +++ b/src/libosmo-mgcp/mgcp_protocol.c @@ -666,10 +666,11 @@ if (p->cfg->change_cb) p->cfg->change_cb(tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_CRCX); + /* Send dummy packet, see also comments in mgcp_keepalive_timer_cb() */ + OSMO_ASSERT(tcfg->keepalive_interval >= MGCP_KEEPALIVE_ONCE); if (conn->conn->mode & MGCP_CONN_RECV_ONLY - && tcfg->keepalive_interval != 0) { + && tcfg->keepalive_interval != MGCP_KEEPALIVE_NEVER) send_dummy(endp, conn); - } LOGP(DLMGCP, LOGL_NOTICE, "CRCX: endpoint:%x connection successfully created\n", @@ -815,8 +816,10 @@ p->cfg->change_cb(endp->tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_MDCX); - if (conn->conn->mode & MGCP_CONN_RECV_ONLY && - endp->tcfg->keepalive_interval != 0) + /* Send dummy packet, see also comments in mgcp_keepalive_timer_cb() */ + OSMO_ASSERT(endp->tcfg->keepalive_interval >= MGCP_KEEPALIVE_ONCE); + if (conn->conn->mode & MGCP_CONN_RECV_ONLY + && endp->tcfg->keepalive_interval != MGCP_KEEPALIVE_NEVER) send_dummy(endp, conn); if (silent) @@ -1051,10 +1054,25 @@ struct mgcp_conn *conn; int i; - LOGP(DLMGCP, LOGL_DEBUG, "Triggered trunk %d keepalive timer.\n", + LOGP(DLMGCP, LOGL_DEBUG, "triggered trunk %d keepalive timer\n", tcfg->trunk_nr); - if (tcfg->keepalive_interval <= 0) + /* Do not accept invalid configuration values + * valid is MGCP_KEEPALIVE_NEVER, MGCP_KEEPALIVE_ONCE and + * values greater 0 */ + OSMO_ASSERT(tcfg->keepalive_interval >= MGCP_KEEPALIVE_ONCE); + + /* The dummy packet functionality has been disabled, we will exit + * immediately, no further timer is scheduled, which means we will no + * longer send dummy packets even when we did before */ + if (tcfg->keepalive_interval == MGCP_KEEPALIVE_NEVER) + return; + + /* In cases where only one dummy packet is sent, we do not need + * the timer since the functions that handle the CRCX and MDCX are + * triggering the sending of the dummy packet. So we behave like in + * the MGCP_KEEPALIVE_NEVER case */ + if (tcfg->keepalive_interval == MGCP_KEEPALIVE_ONCE) return; /* Send walk over all endpoints and send out dummy packets through @@ -1067,7 +1085,8 @@ } } - LOGP(DLMGCP, LOGL_DEBUG, "Rescheduling trunk %d keepalive timer.\n", + /* Schedule the keepalive timer for the next round */ + LOGP(DLMGCP, LOGL_DEBUG, "rescheduling trunk %d keepalive timer\n", tcfg->trunk_nr); osmo_timer_schedule(&tcfg->keepalive_timer, tcfg->keepalive_interval, 0); diff --git a/src/libosmo-mgcp/mgcp_vty.c b/src/libosmo-mgcp/mgcp_vty.c index e8ad818..7107bcc 100644 --- a/src/libosmo-mgcp/mgcp_vty.c +++ b/src/libosmo-mgcp/mgcp_vty.c @@ -569,7 +569,7 @@ cfg_mgcp_no_rtp_keepalive_cmd, "no rtp keep-alive", NO_STR RTP_STR RTP_KEEPALIVE_STR) { - mgcp_trunk_set_keepalive(&g_cfg->trunk, 0); + mgcp_trunk_set_keepalive(&g_cfg->trunk, MGCP_KEEPALIVE_NEVER); return CMD_SUCCESS; } -- To view, visit https://gerrit.osmocom.org/4617 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7ee9409baec50a09fb357d655b5253434fae924 Gerrit-PatchSet: 3 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de>