Harald Welte has submitted this change and it was merged.

Change subject: cosmetic: make dummy packet handling more explicit
......................................................................


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(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



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: merged
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>

Reply via email to