[PATCH 1/9] mgcp/rtp: Refactor mgcp_send to avoid code duplication
Currently there are two symmetric code paths which are selected by the packet destination (NET or BTS). This patch introduces 3 variables that take the different values and unifies both code paths into one. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 50 ++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 657019e..1c7c3da 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -537,6 +537,10 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, struct sockaddr_in *addr, char *buf, int rc) { struct mgcp_trunk_config *tcfg = endp->tcfg; + struct mgcp_rtp_end *rtp_end; + struct mgcp_rtp_state *rtp_state; + int tap_idx; + /* For loop toggle the destination and then dispatch. */ if (tcfg->audio_loop) dest = !dest; @@ -546,35 +550,25 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, dest = !dest; if (dest == MGCP_DEST_NET) { - if (is_rtp) { - mgcp_patch_and_count(endp, &endp->bts_state, - &endp->net_end, - addr, buf, rc); - forward_data(endp->net_end.rtp.fd, - &endp->taps[MGCP_TAP_NET_OUT], buf, rc); - return mgcp_udp_send(endp->net_end.rtp.fd, - &endp->net_end.addr, - endp->net_end.rtp_port, buf, rc); - } else if (!tcfg->omit_rtcp) { - return mgcp_udp_send(endp->net_end.rtcp.fd, - &endp->net_end.addr, - endp->net_end.rtcp_port, buf, rc); - } + rtp_end = &endp->net_end; + rtp_state = &endp->bts_state; + tap_idx = MGCP_TAP_NET_OUT; } else { - if (is_rtp) { - mgcp_patch_and_count(endp, &endp->net_state, - &endp->bts_end, - addr, buf, rc); - forward_data(endp->bts_end.rtp.fd, - &endp->taps[MGCP_TAP_BTS_OUT], buf, rc); - return mgcp_udp_send(endp->bts_end.rtp.fd, - &endp->bts_end.addr, - endp->bts_end.rtp_port, buf, rc); - } else if (!tcfg->omit_rtcp) { - return mgcp_udp_send(endp->bts_end.rtcp.fd, - &endp->bts_end.addr, - endp->bts_end.rtcp_port, buf, rc); - } + rtp_end = &endp->bts_end; + rtp_state = &endp->net_state; + tap_idx = MGCP_TAP_BTS_OUT; + } + + if (is_rtp) { + mgcp_patch_and_count(endp, rtp_state, rtp_end, addr, buf, rc); + forward_data(rtp_end->rtp.fd, &endp->taps[tap_idx], buf, rc); + return mgcp_udp_send(rtp_end->rtp.fd, + &rtp_end->addr, + rtp_end->rtp_port, buf, rc); + } else if (!tcfg->omit_rtcp) { + return mgcp_udp_send(rtp_end->rtcp.fd, + &rtp_end->addr, + rtp_end->rtcp_port, buf, rc); } return 0; -- 1.7.9.5
This patch make it possible to have a valid endpoint that drops all outgoing RTP packets. The number of dropped packets is shown by the VTY 'show mgcp' command. By default, this feature is disabled. To enable packet dropping, the corresponding output_enabled field must be set to 0. Ticket: OW#1044 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp_internal.h | 2 ++ openbsc/src/libmgcp/mgcp_network.c | 4 +++- openbsc/src/libmgcp/mgcp_protocol.c | 2 ++ openbsc/src/libmgcp/mgcp_vty.c | 8 +++++++- 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/openbsc/include/openbsc/mgcp_internal.h b/openbsc/include/openbsc/mgcp_internal.h index a9ae33c..b4899e4 100644 --- a/openbsc/include/openbsc/mgcp_internal.h +++ b/openbsc/include/openbsc/mgcp_internal.h @@ -71,6 +71,7 @@ struct mgcp_rtp_end { /* statistics */ unsigned int packets; unsigned int octets; + unsigned int dropped_packets; struct in_addr addr; /* in network byte order */ @@ -84,6 +85,7 @@ struct mgcp_rtp_end { int frames_per_packet; uint32_t packet_duration_ms; char *fmtp_extra; + int output_enabled; /* RTP patching */ int force_constant_ssrc; /* -1: always, 0: don't, 1: once */ diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 1c7c3da..21d52b5 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -559,7 +559,9 @@ static int mgcp_send(struct mgcp_endpoint *endp, int dest, int is_rtp, tap_idx = MGCP_TAP_BTS_OUT; } - if (is_rtp) { + if (!rtp_end->output_enabled) + rtp_end->dropped_packets += 1; + else if (is_rtp) { mgcp_patch_and_count(endp, rtp_state, rtp_end, addr, buf, rc); forward_data(rtp_end->rtp.fd, &endp->taps[tap_idx], buf, rc); return mgcp_udp_send(rtp_end->rtp.fd, diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index ddec44d..95f3910 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -1178,6 +1178,7 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->packets = 0; end->octets = 0; + end->dropped_packets = 0; memset(&end->addr, 0, sizeof(end->addr)); end->rtp_port = end->rtcp_port = 0; end->payload_type = -1; @@ -1191,6 +1192,7 @@ static void mgcp_rtp_end_reset(struct mgcp_rtp_end *end) end->frames_per_packet = 0; /* unknown */ end->packet_duration_ms = DEFAULT_RTP_AUDIO_PACKET_DURATION_MS; end->rate = DEFAULT_RTP_AUDIO_DEFAULT_RATE; + end->output_enabled = 1; } static void mgcp_rtp_end_init(struct mgcp_rtp_end *end) diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 8411b4a..3f1ebeb 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -150,7 +150,7 @@ static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg, int verbo endp->trans_net.packets, endp->trans_bts.packets, VTY_NEWLINE); - if (verbose) + if (verbose) { vty_out(vty, " Timestamp Errs: BTS %d->%d, Net %d->%d%s", endp->bts_state.in_stream.err_ts_counter, @@ -158,6 +158,12 @@ static void dump_trunk(struct vty *vty, struct mgcp_trunk_config *cfg, int verbo endp->net_state.in_stream.err_ts_counter, endp->net_state.out_stream.err_ts_counter, VTY_NEWLINE); + vty_out(vty, + " Dropped Packets: Net->BTS %d, BTS->Net %d%s", + endp->bts_end.dropped_packets, + endp->net_end.dropped_packets, + VTY_NEWLINE); + } } } -- 1.7.9.5
This modifies the test MGCP messages to include M:recvonly (CRCX), M:sendrecv (MDCX) and M:sendonly (MDCX) outputs the resulting output_enabled flags and the conn_mode after processing the message. Sponsored-by: On-Waves ehf --- openbsc/tests/mgcp/mgcp_test.c | 38 ++++++++++++++++++++++++++++++++++++-- openbsc/tests/mgcp/mgcp_test.ok | 15 ++++++++++++++- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 3cfc183..7653a0d 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -84,6 +84,7 @@ static void test_strline(void) "a=rtpmap:126 AMR/8000\r\n" \ "a=ptime:20\r\n" #define MDCX4 "MDCX 18983216 1@mgw MGCP 1.0\r\n" \ + "M: sendrecv\r" \ "C: 2\r\n" \ "I: 1\r\n" \ "L: p:20, a:AMR, nt:IN\r\n" \ @@ -107,6 +108,7 @@ static void test_strline(void) "a=ptime:20\r\n" #define MDCX4_PT1 "MDCX 18983217 1@mgw MGCP 1.0\r\n" \ + "M: sendrecv\r" \ "C: 2\r\n" \ "I: 1\r\n" \ "L: p:20-40, a:AMR, nt:IN\r\n" \ @@ -120,6 +122,7 @@ static void test_strline(void) "a=ptime:40\r\n" #define MDCX4_PT2 "MDCX 18983218 1@mgw MGCP 1.0\r\n" \ + "M: sendrecv\r" \ "C: 2\r\n" \ "I: 1\r\n" \ "L: p:20-20, a:AMR, nt:IN\r\n" \ @@ -133,6 +136,7 @@ static void test_strline(void) "a=ptime:40\r\n" #define MDCX4_PT3 "MDCX 18983219 1@mgw MGCP 1.0\r\n" \ + "M: sendrecv\r" \ "C: 2\r\n" \ "I: 1\r\n" \ "L: a:AMR, nt:IN\r\n" \ @@ -145,6 +149,20 @@ static void test_strline(void) "a=rtpmap:99 AMR/8000\r\n" \ "a=ptime:40\r\n" +#define MDCX4_SO "MDCX 18983220 1@mgw MGCP 1.0\r\n" \ + "M: sendonly\r" \ + "C: 2\r\n" \ + "I: 1\r\n" \ + "L: p:20, a:AMR, nt:IN\r\n" \ + "\n" \ + "v=0\r\n" \ + "o=- 1 23 IN IP4 0.0.0.0\r\n" \ + "c=IN IP4 0.0.0.0\r\n" \ + "t=0 0\r\n" \ + "m=audio 4441 RTP/AVP 99\r\n" \ + "a=rtpmap:99 AMR/8000\r\n" \ + "a=ptime:40\r\n" + #define SHORT2 "CRCX 1" #define SHORT2_RET "510 000000 FAIL\r\n" #define SHORT3 "CRCX 1 1@mgw" @@ -152,7 +170,7 @@ static void test_strline(void) #define SHORT5 "CRCX 1 1@mgw MGCP 1.0" #define CRCX "CRCX 2 1@mgw MGCP 1.0\r\n" \ - "M: sendrecv\r\n" \ + "M: recvonly\r\n" \ "C: 2\r\n" \ "L: p:20\r\n" \ "\r\n" \ @@ -174,7 +192,7 @@ static void test_strline(void) "a=ptime:20\r\n" #define CRCX_ZYN "CRCX 2 1@mgw MGCP 1.0\r" \ - "M: sendrecv\r" \ + "M: recvonly\r" \ "C: 2\r\r" \ "v=0\r" \ "c=IN IP4 123.12.12.123\r" \ @@ -233,6 +251,7 @@ static const struct mgcp_test tests[] = { { "MDCX4_PT1", MDCX4_PT1, MDCX4_RET("18983217"), 99, 126 }, { "MDCX4_PT2", MDCX4_PT2, MDCX4_RET("18983218"), 99, 126 }, { "MDCX4_PT3", MDCX4_PT3, MDCX4_RET("18983219"), 99, 126 }, + { "MDCX4_SO", MDCX4_SO, MDCX4_RET("18983220"), 99, 126 }, { "DLCX", DLCX, DLCX_RET, -1, -1 }, { "CRCX_ZYN", CRCX_ZYN, CRCX_ZYN_RET, 97, 126 }, { "EMPTY", EMPTY, EMPTY_RET }, @@ -294,6 +313,9 @@ static void test_messages(void) endp = &cfg->trunk.endpoints[i]; endp->net_end.payload_type = PTYPE_NONE; endp->net_end.packet_duration_ms = -1; + endp->bts_end.output_enabled = 0; + endp->net_end.output_enabled = 0; + endp->conn_mode = -1; } for (i = 0; i < ARRAY_SIZE(tests); i++) { @@ -332,7 +354,19 @@ static void test_messages(void) else printf("Requested packetization period not set\n"); + if (endp->conn_mode != -1) + printf("Connection mode: %d, " + "BTS output %sabled, NET output %sabled\n", + endp->conn_mode, + endp->bts_end.output_enabled ? "en" : "dis", + endp->net_end.output_enabled ? "en" : "dis"); + else + printf("Connection mode not set\n"); + endp->net_end.packet_duration_ms = -1; + endp->bts_end.output_enabled = 0; + endp->net_end.output_enabled = 0; + endp->conn_mode = -1; } diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 638ac92..d5fb56f 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -18,27 +18,39 @@ Testing MDCX2 Testing CRCX Detected packet duration: 40 Requested packetetization period: 20-20 +Connection mode: 1, BTS output disabled, NET output disabled Testing MDCX3 Packet duration not set Requested packetization period not set +Connection mode not set Testing MDCX4 Detected packet duration: 40 Requested packetetization period: 20-20 +Connection mode: 3, BTS output disabled, NET output disabled Testing MDCX4_PT1 Detected packet duration: 40 Requested packetetization period: 20-40 +Connection mode: 3, BTS output disabled, NET output disabled Testing MDCX4_PT2 Detected packet duration: 40 Requested packetetization period: 20-20 +Connection mode: 3, BTS output disabled, NET output disabled Testing MDCX4_PT3 Detected packet duration: 40 Requested packetization period not set +Connection mode: 3, BTS output disabled, NET output disabled +Testing MDCX4_SO +Detected packet duration: 40 +Requested packetetization period: 20-20 +Connection mode: 2, BTS output disabled, NET output disabled Testing DLCX Detected packet duration: 20 -Requested packetization period not set +Requested packetetization period: 20-20 +Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX_ZYN Packet duration not set Requested packetization period not set +Connection mode: 1, BTS output disabled, NET output disabled Testing EMPTY Testing SHORT1 Testing SHORT2 @@ -49,6 +61,7 @@ Testing RQNT2 Testing DLCX Detected packet duration: 20 Requested packetization period not set +Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX Re-transmitting CRCX Testing RQNT1 -- 1.7.9.5
On Tue, Jan 07, 2014 at 12:07:45PM +0100, Jacob Erlbeck wrote:
+ endp->bts_end.output_enabled = 0; + endp->net_end.output_enabled = 0;
+ endp->bts_end.output_enabled = 0; + endp->net_end.output_enabled = 0;
+Connection mode: 1, BTS output disabled, NET output disabled
It is the third time I try to write something but I don't find the right words. I am afraid that with the manipulation of internal state we move away from testing the real thing. I understand that you want to capture the effect of parsing the message once. What about assigning variables that are not within the scope of what libmgcp/ would normally use? This way we can be sure that a.) libmgcp set these variables and b.) they are what we wanted it to be set to? holger
On 13.01.2014 09:52, Holger Hans Peter Freyther wrote:
On Tue, Jan 07, 2014 at 12:07:45PM +0100, Jacob Erlbeck wrote:
+ endp->bts_end.output_enabled = 0; + endp->net_end.output_enabled = 0;
I am afraid that with the manipulation of internal state we move away from testing the real thing.
Hmm, that's unit testing mainly targeted at the MGCP parser. So I'm not too concerned about that, as long as I keep away from the part of the state that is really used by the parser and not only set as a side effect.
I understand that you want to capture the effect of parsing the message once. What about assigning variables that are not within the scope of what libmgcp/ would normally use? This way we can be sure that a.) libmgcp set these variables and b.) they are what we wanted it to be set to?
That would be ok, what's about 42 or -1? Jacob
Currently these value may leak into the next test. This patch will reset them after each message has been processed. Sponsored-by: On-Waves ehf --- openbsc/tests/mgcp/mgcp_test.c | 2 ++ openbsc/tests/mgcp/mgcp_test.ok | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 7653a0d..538dc9f 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -366,6 +366,8 @@ static void test_messages(void) endp->net_end.packet_duration_ms = -1; endp->bts_end.output_enabled = 0; endp->net_end.output_enabled = 0; + endp->local_options.pkt_period_min = 0; + endp->local_options.pkt_period_max = 0; endp->conn_mode = -1; } diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index d5fb56f..43a7402 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -45,7 +45,7 @@ Requested packetetization period: 20-20 Connection mode: 2, BTS output disabled, NET output disabled Testing DLCX Detected packet duration: 20 -Requested packetetization period: 20-20 +Requested packetization period not set Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX_ZYN Packet duration not set -- 1.7.9.5
This patch extends the test_messages test to show the number of dummy packets that has been passed to sendto(). This way, dummy packets are even being counted when sendto() itself failes (e.g. due to the fd being -1). Sponsored-by: On-Waves ehf --- openbsc/tests/mgcp/mgcp_test.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/openbsc/tests/mgcp/mgcp_test.c b/openbsc/tests/mgcp/mgcp_test.c index 538dc9f..7eeef99 100644 --- a/openbsc/tests/mgcp/mgcp_test.c +++ b/openbsc/tests/mgcp/mgcp_test.c @@ -16,6 +16,8 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#undef _GNU_SOURCE +#define _GNU_SOURCE #include <openbsc/mgcp.h> #include <openbsc/mgcp_internal.h> @@ -24,6 +26,7 @@ #include <osmocom/core/talloc.h> #include <string.h> #include <limits.h> +#include <dlfcn.h> char *strline_r(char *str, char **saveptr); @@ -293,6 +296,31 @@ static int mgcp_test_policy_cb(struct mgcp_trunk_config *cfg, int endpoint, return MGCP_POLICY_CONT; } +#define MGCP_DUMMY_LOAD 0x23 +static int dummy_packets = 0; +/* override and forward */ +ssize_t sendto(int sockfd, const void *buf, size_t len, int flags, + const struct sockaddr *dest_addr, socklen_t addrlen) +{ + typedef ssize_t (*sendto_t)(int, const void *, size_t, int, + const struct sockaddr *, socklen_t); + static sendto_t real_sendto = NULL; + uint32_t dest_host = htonl(((struct sockaddr_in *)dest_addr)->sin_addr.s_addr); + int dest_port = htons(((struct sockaddr_in *)dest_addr)->sin_port); + + if (!real_sendto) + real_sendto = dlsym(RTLD_NEXT, "sendto"); + + if (len == 1 && ((const char *)buf)[0] == MGCP_DUMMY_LOAD ) { + fprintf(stderr, "Dummy packet to 0x%08x:%d, msg length %d\n%s\n\n", + dest_host, dest_port, + len, osmo_hexdump(buf, len)); + dummy_packets += 1; + } + + return real_sendto(sockfd, buf, len, flags, dest_addr, addrlen); +} + static void test_messages(void) { struct mgcp_config *cfg; @@ -326,6 +354,7 @@ static void test_messages(void) printf("Testing %s\n", t->name); last_endpoint = -1; + dummy_packets = 0; inp = create_msg(t->req); msg = mgcp_handle_message(cfg, inp); @@ -337,6 +366,9 @@ static void test_messages(void) printf("%s failed '%s'\n", t->name, (char *) msg->data); msgb_free(msg); + if (dummy_packets) + printf("Dummy packets: %d\n", dummy_packets); + if (last_endpoint != -1) { endp = &cfg->trunk.endpoints[last_endpoint]; -- 1.7.9.5
On Tue, Jan 07, 2014 at 12:07:47PM +0100, Jacob Erlbeck wrote: I added: diff --git a/openbsc/tests/mgcp/Makefile.am b/openbsc/tests/mgcp/Makefile.am index 8c365b4..470cdd8 100644 --- a/openbsc/tests/mgcp/Makefile.am +++ b/openbsc/tests/mgcp/Makefile.am @@ -11,4 +11,5 @@ mgcp_test_SOURCES = mgcp_test.c mgcp_test_LDADD = $(top_builddir)/src/libbsc/libbsc.a \ $(top_builddir)/src/libmgcp/libmgcp.a \ $(top_builddir)/src/libcommon/libcommon.a \ - $(LIBOSMOCORE_LIBS) -lrt $(LIBOSMOSCCP_LIBS) $(LIBOSMOVTY_LIBS) + $(LIBOSMOCORE_LIBS) -lrt $(LIBOSMOSCCP_LIBS) $(LIBOSMOVTY_LIBS) \ + $(LIBRARY_DL) for the dlsym
This patch enhances parse_conn_mode() to set the output_enabled flags of each end based on the MGCP mode. Ticket: OW#1044 Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_protocol.c | 37 ++++++++++++++++++++++++++++------- openbsc/tests/mgcp/mgcp_test.ok | 14 ++++++------- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index 95f3910..b55da37 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -488,22 +488,45 @@ static struct msgb *handle_audit_endpoint(struct mgcp_parse_data *p) return create_ok_response(p->endp, 200, "AUEP", p->trans); } -static int parse_conn_mode(const char *msg, int *conn_mode) +static int parse_conn_mode(const char *msg, struct mgcp_endpoint *endp) { int ret = 0; if (strcmp(msg, "recvonly") == 0) - *conn_mode = MGCP_CONN_RECV_ONLY; + endp->conn_mode = MGCP_CONN_RECV_ONLY; else if (strcmp(msg, "sendrecv") == 0) - *conn_mode = MGCP_CONN_RECV_SEND; + endp->conn_mode = MGCP_CONN_RECV_SEND; else if (strcmp(msg, "sendonly") == 0) - *conn_mode = MGCP_CONN_SEND_ONLY; + endp->conn_mode = MGCP_CONN_SEND_ONLY; else if (strcmp(msg, "loopback") == 0) - *conn_mode = MGCP_CONN_LOOPBACK; + endp->conn_mode = MGCP_CONN_LOOPBACK; else { LOGP(DMGCP, LOGL_ERROR, "Unknown connection mode: '%s'\n", msg); ret = -1; } + switch (endp->conn_mode) { + case MGCP_CONN_NONE: + endp->net_end.output_enabled = 0; + endp->bts_end.output_enabled = 0; + break; + + case MGCP_CONN_RECV_ONLY: + endp->net_end.output_enabled = 0; + endp->bts_end.output_enabled = 1; + break; + + case MGCP_CONN_SEND_ONLY: + endp->net_end.output_enabled = 1; + endp->bts_end.output_enabled = 0; + break; + + default: + endp->net_end.output_enabled = 1; + endp->bts_end.output_enabled = 1; + break; + } + + return ret; } @@ -794,7 +817,7 @@ mgcp_header_done: set_local_cx_options(endp->tcfg->endpoints, &endp->local_options, local_options); - if (parse_conn_mode(mode, &endp->conn_mode) != 0) { + if (parse_conn_mode(mode, endp) != 0) { error_code = 517; goto error2; } @@ -895,7 +918,7 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) local_options = (const char *) line + 3; break; case 'M': - if (parse_conn_mode(line + 3, &endp->conn_mode) != 0) { + if (parse_conn_mode(line + 3, endp) != 0) { error_code = 517; goto error3; } diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 43a7402..6db7226 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -18,7 +18,7 @@ Testing MDCX2 Testing CRCX Detected packet duration: 40 Requested packetetization period: 20-20 -Connection mode: 1, BTS output disabled, NET output disabled +Connection mode: 1, BTS output enabled, NET output disabled Testing MDCX3 Packet duration not set Requested packetization period not set @@ -26,23 +26,23 @@ Connection mode not set Testing MDCX4 Detected packet duration: 40 Requested packetetization period: 20-20 -Connection mode: 3, BTS output disabled, NET output disabled +Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT1 Detected packet duration: 40 Requested packetetization period: 20-40 -Connection mode: 3, BTS output disabled, NET output disabled +Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT2 Detected packet duration: 40 Requested packetetization period: 20-20 -Connection mode: 3, BTS output disabled, NET output disabled +Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT3 Detected packet duration: 40 Requested packetization period not set -Connection mode: 3, BTS output disabled, NET output disabled +Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_SO Detected packet duration: 40 Requested packetetization period: 20-20 -Connection mode: 2, BTS output disabled, NET output disabled +Connection mode: 2, BTS output disabled, NET output enabled Testing DLCX Detected packet duration: 20 Requested packetization period not set @@ -50,7 +50,7 @@ Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX_ZYN Packet duration not set Requested packetization period not set -Connection mode: 1, BTS output disabled, NET output disabled +Connection mode: 1, BTS output enabled, NET output disabled Testing EMPTY Testing SHORT1 Testing SHORT2 -- 1.7.9.5
On Tue, Jan 07, 2014 at 12:07:48PM +0100, Jacob Erlbeck wrote: Good Morning,
diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 43a7402..6db7226 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok
In the test case you force the output_enabled variable to 0. Now that the state is set based on the connection mode I think it is dangerous to set the state inside the testcase. mgcp_free_endpoint should reset it and we should rely and test that. What do you think?
On 08.01.2014 10:45, Holger Hans Peter Freyther wrote:
On Tue, Jan 07, 2014 at 12:07:48PM +0100, Jacob Erlbeck wrote:
Good Morning,
diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 43a7402..6db7226 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok
In the test case you force the output_enabled variable to 0. Now that the state is set based on the connection mode I think it is dangerous to set the state inside the testcase. mgcp_free_endpoint should reset it and we should rely and test that.
What do you think?
We cannot use mgcp_free_endp() there without additional changes in the testing framework, since this would clear flags like ci too, thus breaking e.g. MDCX handling. The order of the test messages is relevant and part of the state must be kept. In addition, mgcp_free_endp() initialises the output_enabled flags to 1 which would make it difficult to check whether sendrecv has been detected successfully. Of course this raises the question, whether it in sensible to set them to 1 at initialisation. While it maintains semantic compatibility, these flags are not in sync with conn_mode after init. Jacob
On Wed, Jan 08, 2014 at 01:06:56PM +0100, Jacob Erlbeck wrote: Good Morning!
We cannot use mgcp_free_endp() there without additional changes in the testing framework, since this would clear flags like ci too, thus breaking e.g. MDCX handling. The order of the test messages is relevant and part of the state must be kept.
That is a valid argument. On the other hand the further we go away from the normal allocation/init procedure of an endpoint the more we test something that does not relate to the real world. What other state do we rely on besides the CI?
In addition, mgcp_free_endp() initialises the output_enabled flags to 1 which would make it difficult to check whether sendrecv has been detected successfully. Of course this raises the question, whether it in sensible to set them to 1 at initialisation. While it maintains semantic compatibility, these flags are not in sync with conn_mode after init.
That is true. I have a local refactoring backlog to make mgcp_free_endp free some resources and have a separate mgcp_init_endp to initialize it just after the allocation. :}
Rename the timestamp variable to make in clear, that the input timestamp is meant. Add a helper variable to illustrate the offset computation. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 21d52b5..04e6cbe 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -241,10 +241,11 @@ static int adjust_rtp_timestamp_offset(struct mgcp_endpoint *endp, struct mgcp_rtp_state *state, struct mgcp_rtp_end *rtp_end, struct sockaddr_in *addr, - int16_t delta_seq, uint32_t timestamp) + int16_t delta_seq, uint32_t in_timestamp) { int32_t tsdelta = state->packet_duration; int timestamp_offset; + uint32_t out_timestamp; if (tsdelta == 0) { tsdelta = state->out_stream.last_tsdelta; @@ -269,9 +270,8 @@ static int adjust_rtp_timestamp_offset(struct mgcp_endpoint *endp, } } - timestamp_offset = - state->out_stream.last_timestamp - timestamp + - delta_seq * tsdelta; + out_timestamp = state->out_stream.last_timestamp + delta_seq * tsdelta; + timestamp_offset = out_timestamp - in_timestamp; if (state->timestamp_offset != timestamp_offset) { state->timestamp_offset = timestamp_offset; -- 1.7.9.5
Currently a dummy packet is only sent to the RTP port. This is not enough if RTCP must also cross the SNAT. This patch sends an additional dummy packet to the RTCP net destination if omit_rtcp is not set. Sponsored-by: On-Waves ehf --- openbsc/src/libmgcp/mgcp_network.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/openbsc/src/libmgcp/mgcp_network.c b/openbsc/src/libmgcp/mgcp_network.c index 04e6cbe..0cc2041 100644 --- a/openbsc/src/libmgcp/mgcp_network.c +++ b/openbsc/src/libmgcp/mgcp_network.c @@ -129,9 +129,16 @@ static int mgcp_udp_send(int fd, struct in_addr *addr, int port, char *buf, int mgcp_send_dummy(struct mgcp_endpoint *endp) { static char buf[] = { MGCP_DUMMY_LOAD }; + int rc; + + rc = mgcp_udp_send(endp->net_end.rtp.fd, &endp->net_end.addr, + endp->net_end.rtp_port, buf, 1); + + if (rc == -1 || endp->tcfg->omit_rtcp) + return rc; - return mgcp_udp_send(endp->net_end.rtp.fd, &endp->net_end.addr, - endp->net_end.rtp_port, buf, 1); + return mgcp_udp_send(endp->net_end.rtcp.fd, &endp->net_end.addr, + endp->net_end.rtcp_port, buf, 1); } static int32_t compute_timestamp_aligment_error(struct mgcp_rtp_stream_state *sstate, -- 1.7.9.5
So far, a single dummy packet has been sent immediately after the reception of a MDCX message. There is no dedicated keep alive mechanism (it just worked because the audio from the MS has always been forwarded to the NAT until the 'mgcp: Set output_enabled flags based on the MGCP mode' patch). This patch adds explicit, timer based keep alive handling that can be enable per trunk. A VTY command 'rtp keep-alive' command is added for configuration which can be used to set the interval in seconds, to send a single packet after the reception of a CRCX/MDCX when RTP data from the net is expected ('once'), or to disable the feature completely ('no rtp keep-alive'). The default is 'once'. Note that this removes the mgcp_change_cb() from mgcp_main.c. Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp.h | 9 ++++ openbsc/src/libmgcp/mgcp_protocol.c | 43 +++++++++++++++++ openbsc/src/libmgcp/mgcp_vty.c | 86 +++++++++++++++++++++++++++++++++ openbsc/src/osmo-bsc_mgcp/mgcp_main.c | 10 ---- openbsc/tests/mgcp/mgcp_test.ok | 6 +++ 5 files changed, 144 insertions(+), 10 deletions(-) diff --git a/openbsc/include/openbsc/mgcp.h b/openbsc/include/openbsc/mgcp.h index 335c83d..1d74078 100644 --- a/openbsc/include/openbsc/mgcp.h +++ b/openbsc/include/openbsc/mgcp.h @@ -25,6 +25,7 @@ #include <osmocom/core/msgb.h> #include <osmocom/core/write_queue.h> +#include <osmocom/core/timer.h> #include "debug.h" @@ -103,6 +104,8 @@ struct mgcp_port_range { int last_port; }; +#define MGCP_KEEPALIVE_ONCE (-1) + struct mgcp_trunk_config { struct llist_head entry; @@ -118,6 +121,7 @@ struct mgcp_trunk_config { int audio_loop; int omit_rtcp; + int keepalive_interval; /* RTP patching */ int force_constant_ssrc; /* 0: don't, 1: once */ @@ -126,6 +130,9 @@ struct mgcp_trunk_config { /* spec handling */ int force_realloc; + /* timer */ + struct osmo_timer_list keepalive_timer; + unsigned int number_endpoints; struct mgcp_endpoint *endpoints; }; @@ -187,6 +194,8 @@ int mgcp_reset_transcoder(struct mgcp_config *cfg); void mgcp_format_stats(struct mgcp_endpoint *endp, char *stats, size_t size); int mgcp_parse_stats(struct msgb *msg, uint32_t *ps, uint32_t *os, uint32_t *pr, uint32_t *_or, int *loss, uint32_t *jitter); +void mgcp_trunk_set_keepalive(struct mgcp_trunk_config *tcfg, int interval); + /* * format helper functions */ diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index b55da37..8f6b7c2 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -877,6 +877,9 @@ mgcp_header_done: if (p->cfg->change_cb) p->cfg->change_cb(tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_CRCX); + if (endp->bts_end.output_enabled && tcfg->keepalive_interval != 0) + mgcp_send_dummy(endp); + create_transcoder(endp); return create_response_with_sdp(endp, "CRCX", p->trans); error2: @@ -975,6 +978,10 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) ENDPOINT_NUMBER(endp), inet_ntoa(endp->net_end.addr), ntohs(endp->net_end.rtp_port)); if (p->cfg->change_cb) p->cfg->change_cb(endp->tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_MDCX); + + if (endp->bts_end.output_enabled && endp->tcfg->keepalive_interval != 0) + mgcp_send_dummy(endp); + if (silent) goto out_silent; @@ -1127,6 +1134,40 @@ static struct msgb *handle_noti_req(struct mgcp_parse_data *p) create_err_response(p->endp, res, "RQNT", p->trans); } +static void mgcp_keepalive_timer_cb(void *_tcfg) +{ + struct mgcp_trunk_config *tcfg = _tcfg; + int i; + LOGP(DMGCP, LOGL_DEBUG, "Triggered trunk %d keepalive timer.\n", + tcfg->trunk_nr); + + if (tcfg->keepalive_interval <= 0) + return; + + for (i = 1; i < tcfg->number_endpoints; ++i) { + struct mgcp_endpoint *endp = &tcfg->endpoints[i]; + if (endp->bts_end.output_enabled) + mgcp_send_dummy(endp); + } + + LOGP(DMGCP, LOGL_DEBUG, "Rescheduling trunk %d keepalive timer.\n", + tcfg->trunk_nr); + osmo_timer_schedule(&tcfg->keepalive_timer, tcfg->keepalive_interval, 0); +} + +void mgcp_trunk_set_keepalive(struct mgcp_trunk_config *tcfg, int interval) +{ + tcfg->keepalive_interval = interval; + tcfg->keepalive_timer.data = tcfg; + tcfg->keepalive_timer.cb = mgcp_keepalive_timer_cb; + + if (interval <= 0) + osmo_timer_del(&tcfg->keepalive_timer); + else + osmo_timer_schedule(&tcfg->keepalive_timer, + tcfg->keepalive_interval, 0); +} + struct mgcp_config *mgcp_config_alloc(void) { struct mgcp_config *cfg; @@ -1153,6 +1194,7 @@ struct mgcp_config *mgcp_config_alloc(void) cfg->trunk.audio_payload = 126; cfg->trunk.audio_send_ptime = 1; cfg->trunk.omit_rtcp = 0; + mgcp_trunk_set_keepalive(&cfg->trunk, MGCP_KEEPALIVE_ONCE); INIT_LLIST_HEAD(&cfg->trunks); @@ -1177,6 +1219,7 @@ struct mgcp_trunk_config *mgcp_trunk_alloc(struct mgcp_config *cfg, int nr) trunk->audio_send_ptime = 1; trunk->number_endpoints = 33; trunk->omit_rtcp = 0; + mgcp_trunk_set_keepalive(trunk, MGCP_KEEPALIVE_ONCE); llist_add_tail(&trunk->entry, &cfg->trunks); return trunk; } diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 3f1ebeb..8004d53 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -32,6 +32,7 @@ #define RTCP_OMIT_STR "Drop RTCP packets in both directions\n" #define RTP_PATCH_STR "Modify RTP packet header in both directions\n" +#define RTP_KEEPALIVE_STR "Send dummy UDP packet to net RTP destination\n" static struct mgcp_config *g_cfg = NULL; @@ -85,6 +86,12 @@ static int config_write_mgcp(struct vty *vty) g_cfg->net_ports.range_start, g_cfg->net_ports.range_end, VTY_NEWLINE); vty_out(vty, " rtp ip-dscp %d%s", g_cfg->endp_dscp, VTY_NEWLINE); + if (g_cfg->trunk.keepalive_interval) + vty_out(vty, " rtp keep-alive %d%s", + g_cfg->trunk.keepalive_interval, VTY_NEWLINE); + else + vty_out(vty, " no rtp keep-alive%s", VTY_NEWLINE); + if (g_cfg->trunk.omit_rtcp) vty_out(vty, " rtcp-omit%s", VTY_NEWLINE); else @@ -511,6 +518,39 @@ DEFUN(cfg_mgcp_no_patch_rtp, return CMD_SUCCESS; } +DEFUN(cfg_mgcp_rtp_keepalive, + cfg_mgcp_rtp_keepalive_cmd, + "rtp keep-alive <1-120>", + RTP_STR RTP_KEEPALIVE_STR + "Keep alive interval in secs\n" + ) +{ + mgcp_trunk_set_keepalive(&g_cfg->trunk, atoi(argv[0])); + return CMD_SUCCESS; +} + +DEFUN(cfg_mgcp_rtp_keepalive_once, + cfg_mgcp_rtp_keepalive_once_cmd, + "rtp keep-alive once", + RTP_STR RTP_KEEPALIVE_STR + "Send keep alive once after CRCX\n" + ) +{ + mgcp_trunk_set_keepalive(&g_cfg->trunk, MGCP_KEEPALIVE_ONCE); + return CMD_SUCCESS; +} + +DEFUN(cfg_mgcp_no_rtp_keepalive, + 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); + return CMD_SUCCESS; +} + + #define CALL_AGENT_STR "Callagent information\n" DEFUN(cfg_mgcp_agent_addr, @@ -598,6 +638,13 @@ static int config_write_trunk(struct vty *vty) trunk->audio_name, VTY_NEWLINE); vty_out(vty, " %ssdp audio-payload send-ptime%s", trunk->audio_send_ptime ? "" : "no ", VTY_NEWLINE); + + if (trunk->keepalive_interval) + vty_out(vty, " rtp keep-alive %d%s", + trunk->keepalive_interval, VTY_NEWLINE); + else + vty_out(vty, " no rtp keep-alive%s", VTY_NEWLINE); + vty_out(vty, " loop %d%s", trunk->audio_loop, VTY_NEWLINE); if (trunk->omit_rtcp) @@ -780,6 +827,39 @@ DEFUN(cfg_trunk_no_patch_rtp, return CMD_SUCCESS; } +DEFUN(cfg_trunk_rtp_keepalive, + cfg_trunk_rtp_keepalive_cmd, + "rtp keep-alive <1-120>", + RTP_STR RTP_KEEPALIVE_STR + "Keepalive interval in secs\n" + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, atoi(argv[0])); + return CMD_SUCCESS; +} + +DEFUN(cfg_trunk_rtp_keepalive_once, + cfg_trunk_rtp_keepalive_once_cmd, + "rtp keep-alive once", + RTP_STR RTP_KEEPALIVE_STR + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, MGCP_KEEPALIVE_ONCE); + return CMD_SUCCESS; +} + +DEFUN(cfg_trunk_no_rtp_keepalive, + cfg_trunk_no_rtp_keepalive_cmd, + "no rtp keep-alive", + NO_STR RTP_STR RTP_KEEPALIVE_STR + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, 0); + return CMD_SUCCESS; +} DEFUN(loop_endp, loop_endp_cmd, @@ -999,6 +1079,9 @@ int mgcp_vty_init(void) install_element(MGCP_NODE, &cfg_mgcp_rtp_transcoder_base_cmd); install_element(MGCP_NODE, &cfg_mgcp_rtp_ip_dscp_cmd); install_element(MGCP_NODE, &cfg_mgcp_rtp_ip_tos_cmd); + install_element(MGCP_NODE, &cfg_mgcp_rtp_keepalive_cmd); + install_element(MGCP_NODE, &cfg_mgcp_rtp_keepalive_once_cmd); + install_element(MGCP_NODE, &cfg_mgcp_no_rtp_keepalive_cmd); install_element(MGCP_NODE, &cfg_mgcp_agent_addr_cmd); install_element(MGCP_NODE, &cfg_mgcp_agent_addr_cmd_old); install_element(MGCP_NODE, &cfg_mgcp_transcoder_cmd); @@ -1024,6 +1107,9 @@ int mgcp_vty_init(void) install_element(MGCP_NODE, &cfg_mgcp_trunk_cmd); install_node(&trunk_node, config_write_trunk); vty_install_default(TRUNK_NODE); + install_element(TRUNK_NODE, &cfg_trunk_rtp_keepalive_cmd); + install_element(TRUNK_NODE, &cfg_trunk_rtp_keepalive_once_cmd); + install_element(TRUNK_NODE, &cfg_trunk_no_rtp_keepalive_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_number_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_name_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_number_cmd_old); diff --git a/openbsc/src/osmo-bsc_mgcp/mgcp_main.c b/openbsc/src/osmo-bsc_mgcp/mgcp_main.c index 596ea8a..14ec221 100644 --- a/openbsc/src/osmo-bsc_mgcp/mgcp_main.c +++ b/openbsc/src/osmo-bsc_mgcp/mgcp_main.c @@ -137,15 +137,6 @@ static int mgcp_rsip_cb(struct mgcp_trunk_config *tcfg) return 0; } -static int mgcp_change_cb(struct mgcp_trunk_config *cfg, int endpoint, int state) -{ - if (state != MGCP_ENDP_MDCX) - return 0; - - mgcp_send_dummy(&cfg->endpoints[endpoint]); - return 0; -} - static int read_call_agent(struct osmo_fd *fd, unsigned int what) { struct sockaddr_in addr; @@ -233,7 +224,6 @@ int main(int argc, char **argv) /* set some callbacks */ cfg->reset_cb = mgcp_rsip_cb; - cfg->change_cb = mgcp_change_cb; /* we need to bind a socket */ if (rc == 0) { diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 6db7226..7e8aa5c 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -16,6 +16,7 @@ Testing AUEP2 Testing MDCX1 Testing MDCX2 Testing CRCX +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 1, BTS output enabled, NET output disabled @@ -24,18 +25,22 @@ Packet duration not set Requested packetization period not set Connection mode not set Testing MDCX4 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT1 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-40 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT2 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT3 +Dummy packets: 1 Detected packet duration: 40 Requested packetization period not set Connection mode: 3, BTS output enabled, NET output enabled @@ -48,6 +53,7 @@ Detected packet duration: 20 Requested packetization period not set Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX_ZYN +Dummy packets: 1 Packet duration not set Requested packetization period not set Connection mode: 1, BTS output enabled, NET output disabled -- 1.7.9.5
On Tue, Jan 07, 2014 at 12:07:51PM +0100, Jacob Erlbeck wrote:
+ for (i = 1; i < tcfg->number_endpoints; ++i) { + struct mgcp_endpoint *endp = &tcfg->endpoints[i]; + if (endp->bts_end.output_enabled)
Shouldn't this code look at the connection mode? E.g. the output is enabled even in SEND_RECV mode and then we certainly don't need to send dummy packets?
On 13.01.2014 09:57, Holger Hans Peter Freyther wrote:
On Tue, Jan 07, 2014 at 12:07:51PM +0100, Jacob Erlbeck wrote:
+ for (i = 1; i < tcfg->number_endpoints; ++i) { + struct mgcp_endpoint *endp = &tcfg->endpoints[i]; + if (endp->bts_end.output_enabled)
Shouldn't this code look at the connection mode? E.g. the output is enabled even in SEND_RECV mode and then we certainly don't need to send dummy packets?
If (and only if) we can rely on incoming packets from any type of BTS when SEND_RECV is used, we could optimize these few packets away. Otherwise I'd rather check, whether packets have been sent to the net since the last timeout and only send a keep alive packet if there were none. Jacob
On Tue, Jan 14, 2014 at 09:41:39AM +0100, Jacob Erlbeck wrote:
If (and only if) we can rely on incoming packets from any type of BTS when SEND_RECV is used, we could optimize these few packets away.
Otherwise I'd rather check, whether packets have been sent to the net since the last timeout and only send a keep alive packet if there were none.
I disagree. Keep things simple and look at the connection state. if (and only if) we start to look into DTX we can consider adding a heuristic that checks the time of the last outgoing RTP packet. holger
On 14.01.2014 12:54, Holger Hans Peter Freyther wrote:
On Tue, Jan 14, 2014 at 09:41:39AM +0100, Jacob Erlbeck wrote:
If (and only if) we can rely on incoming packets from any type of BTS when SEND_RECV is used, we could optimize these few packets away.
Otherwise I'd rather check, whether packets have been sent to the net since the last timeout and only send a keep alive packet if there were none.
I disagree. Keep things simple and look at the connection state. if (and only if) we start to look into DTX we can consider adding a heuristic that checks the time of the last outgoing RTP packet.
Ok, I'm convinced that my solution makes probably no difference, since keep-alive has only worked with the help of the BTS packets so far anyway. And your approach is indeed simpler and doesn't change the behaviour compared to the old inplementation. Jacob
So far, a single dummy packet has been sent immediately after the reception of a MDCX message. There is no dedicated keep alive mechanism (it just worked because the audio from the MS has always been forwarded to the NAT until the 'mgcp: Set output_enabled flags based on the MGCP mode' patch). This patch adds explicit, timer based keep alive handling that can be enable per trunk. A VTY command 'rtp keep-alive' command is added for configuration which can be used to set the interval in seconds, to send a single packet after the reception of a CRCX/MDCX when RTP data from the net is expected ('once'), or to disable the feature completely ('no rtp keep-alive'). In 'send-recv' connections, only the initial packet is sent if enabled (even when an interval has been configured). The default is 'once'. Note that this removes the mgcp_change_cb() from mgcp_main.c. Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/mgcp.h | 9 ++++ openbsc/src/libmgcp/mgcp_protocol.c | 43 ++++++++++++++++ openbsc/src/libmgcp/mgcp_vty.c | 91 +++++++++++++++++++++++++++++++++ openbsc/src/osmo-bsc_mgcp/mgcp_main.c | 10 ---- openbsc/tests/mgcp/mgcp_test.ok | 6 +++ 5 files changed, 149 insertions(+), 10 deletions(-) diff --git a/openbsc/include/openbsc/mgcp.h b/openbsc/include/openbsc/mgcp.h index 335c83d..1d74078 100644 --- a/openbsc/include/openbsc/mgcp.h +++ b/openbsc/include/openbsc/mgcp.h @@ -25,6 +25,7 @@ #include <osmocom/core/msgb.h> #include <osmocom/core/write_queue.h> +#include <osmocom/core/timer.h> #include "debug.h" @@ -103,6 +104,8 @@ struct mgcp_port_range { int last_port; }; +#define MGCP_KEEPALIVE_ONCE (-1) + struct mgcp_trunk_config { struct llist_head entry; @@ -118,6 +121,7 @@ struct mgcp_trunk_config { int audio_loop; int omit_rtcp; + int keepalive_interval; /* RTP patching */ int force_constant_ssrc; /* 0: don't, 1: once */ @@ -126,6 +130,9 @@ struct mgcp_trunk_config { /* spec handling */ int force_realloc; + /* timer */ + struct osmo_timer_list keepalive_timer; + unsigned int number_endpoints; struct mgcp_endpoint *endpoints; }; @@ -187,6 +194,8 @@ int mgcp_reset_transcoder(struct mgcp_config *cfg); void mgcp_format_stats(struct mgcp_endpoint *endp, char *stats, size_t size); int mgcp_parse_stats(struct msgb *msg, uint32_t *ps, uint32_t *os, uint32_t *pr, uint32_t *_or, int *loss, uint32_t *jitter); +void mgcp_trunk_set_keepalive(struct mgcp_trunk_config *tcfg, int interval); + /* * format helper functions */ diff --git a/openbsc/src/libmgcp/mgcp_protocol.c b/openbsc/src/libmgcp/mgcp_protocol.c index b55da37..9996ba7 100644 --- a/openbsc/src/libmgcp/mgcp_protocol.c +++ b/openbsc/src/libmgcp/mgcp_protocol.c @@ -877,6 +877,9 @@ mgcp_header_done: if (p->cfg->change_cb) p->cfg->change_cb(tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_CRCX); + if (endp->bts_end.output_enabled && tcfg->keepalive_interval != 0) + mgcp_send_dummy(endp); + create_transcoder(endp); return create_response_with_sdp(endp, "CRCX", p->trans); error2: @@ -975,6 +978,10 @@ static struct msgb *handle_modify_con(struct mgcp_parse_data *p) ENDPOINT_NUMBER(endp), inet_ntoa(endp->net_end.addr), ntohs(endp->net_end.rtp_port)); if (p->cfg->change_cb) p->cfg->change_cb(endp->tcfg, ENDPOINT_NUMBER(endp), MGCP_ENDP_MDCX); + + if (endp->bts_end.output_enabled && endp->tcfg->keepalive_interval != 0) + mgcp_send_dummy(endp); + if (silent) goto out_silent; @@ -1127,6 +1134,40 @@ static struct msgb *handle_noti_req(struct mgcp_parse_data *p) create_err_response(p->endp, res, "RQNT", p->trans); } +static void mgcp_keepalive_timer_cb(void *_tcfg) +{ + struct mgcp_trunk_config *tcfg = _tcfg; + int i; + LOGP(DMGCP, LOGL_DEBUG, "Triggered trunk %d keepalive timer.\n", + tcfg->trunk_nr); + + if (tcfg->keepalive_interval <= 0) + return; + + for (i = 1; i < tcfg->number_endpoints; ++i) { + struct mgcp_endpoint *endp = &tcfg->endpoints[i]; + if (endp->conn_mode == MGCP_CONN_RECV_ONLY) + mgcp_send_dummy(endp); + } + + LOGP(DMGCP, LOGL_DEBUG, "Rescheduling trunk %d keepalive timer.\n", + tcfg->trunk_nr); + osmo_timer_schedule(&tcfg->keepalive_timer, tcfg->keepalive_interval, 0); +} + +void mgcp_trunk_set_keepalive(struct mgcp_trunk_config *tcfg, int interval) +{ + tcfg->keepalive_interval = interval; + tcfg->keepalive_timer.data = tcfg; + tcfg->keepalive_timer.cb = mgcp_keepalive_timer_cb; + + if (interval <= 0) + osmo_timer_del(&tcfg->keepalive_timer); + else + osmo_timer_schedule(&tcfg->keepalive_timer, + tcfg->keepalive_interval, 0); +} + struct mgcp_config *mgcp_config_alloc(void) { struct mgcp_config *cfg; @@ -1153,6 +1194,7 @@ struct mgcp_config *mgcp_config_alloc(void) cfg->trunk.audio_payload = 126; cfg->trunk.audio_send_ptime = 1; cfg->trunk.omit_rtcp = 0; + mgcp_trunk_set_keepalive(&cfg->trunk, MGCP_KEEPALIVE_ONCE); INIT_LLIST_HEAD(&cfg->trunks); @@ -1177,6 +1219,7 @@ struct mgcp_trunk_config *mgcp_trunk_alloc(struct mgcp_config *cfg, int nr) trunk->audio_send_ptime = 1; trunk->number_endpoints = 33; trunk->omit_rtcp = 0; + mgcp_trunk_set_keepalive(trunk, MGCP_KEEPALIVE_ONCE); llist_add_tail(&trunk->entry, &cfg->trunks); return trunk; } diff --git a/openbsc/src/libmgcp/mgcp_vty.c b/openbsc/src/libmgcp/mgcp_vty.c index 3f1ebeb..408a059 100644 --- a/openbsc/src/libmgcp/mgcp_vty.c +++ b/openbsc/src/libmgcp/mgcp_vty.c @@ -32,6 +32,7 @@ #define RTCP_OMIT_STR "Drop RTCP packets in both directions\n" #define RTP_PATCH_STR "Modify RTP packet header in both directions\n" +#define RTP_KEEPALIVE_STR "Send dummy UDP packet to net RTP destination\n" static struct mgcp_config *g_cfg = NULL; @@ -85,6 +86,14 @@ static int config_write_mgcp(struct vty *vty) g_cfg->net_ports.range_start, g_cfg->net_ports.range_end, VTY_NEWLINE); vty_out(vty, " rtp ip-dscp %d%s", g_cfg->endp_dscp, VTY_NEWLINE); + if (g_cfg->trunk.keepalive_interval == MGCP_KEEPALIVE_ONCE) + vty_out(vty, " rtp keep-alive once%s", VTY_NEWLINE); + else if (g_cfg->trunk.keepalive_interval) + vty_out(vty, " rtp keep-alive %d%s", + g_cfg->trunk.keepalive_interval, VTY_NEWLINE); + else + vty_out(vty, " no rtp keep-alive%s", VTY_NEWLINE); + if (g_cfg->trunk.omit_rtcp) vty_out(vty, " rtcp-omit%s", VTY_NEWLINE); else @@ -511,6 +520,39 @@ DEFUN(cfg_mgcp_no_patch_rtp, return CMD_SUCCESS; } +DEFUN(cfg_mgcp_rtp_keepalive, + cfg_mgcp_rtp_keepalive_cmd, + "rtp keep-alive <1-120>", + RTP_STR RTP_KEEPALIVE_STR + "Keep alive interval in secs\n" + ) +{ + mgcp_trunk_set_keepalive(&g_cfg->trunk, atoi(argv[0])); + return CMD_SUCCESS; +} + +DEFUN(cfg_mgcp_rtp_keepalive_once, + cfg_mgcp_rtp_keepalive_once_cmd, + "rtp keep-alive once", + RTP_STR RTP_KEEPALIVE_STR + "Send dummy packet only once after CRCX/MDCX\n" + ) +{ + mgcp_trunk_set_keepalive(&g_cfg->trunk, MGCP_KEEPALIVE_ONCE); + return CMD_SUCCESS; +} + +DEFUN(cfg_mgcp_no_rtp_keepalive, + 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); + return CMD_SUCCESS; +} + + #define CALL_AGENT_STR "Callagent information\n" DEFUN(cfg_mgcp_agent_addr, @@ -598,6 +640,15 @@ static int config_write_trunk(struct vty *vty) trunk->audio_name, VTY_NEWLINE); vty_out(vty, " %ssdp audio-payload send-ptime%s", trunk->audio_send_ptime ? "" : "no ", VTY_NEWLINE); + + if (trunk->keepalive_interval == MGCP_KEEPALIVE_ONCE) + vty_out(vty, " rtp keep-alive once%s", VTY_NEWLINE); + else if (trunk->keepalive_interval) + vty_out(vty, " rtp keep-alive %d%s", + trunk->keepalive_interval, VTY_NEWLINE); + else + vty_out(vty, " no rtp keep-alive%s", VTY_NEWLINE); + vty_out(vty, " loop %d%s", trunk->audio_loop, VTY_NEWLINE); if (trunk->omit_rtcp) @@ -780,6 +831,40 @@ DEFUN(cfg_trunk_no_patch_rtp, return CMD_SUCCESS; } +DEFUN(cfg_trunk_rtp_keepalive, + cfg_trunk_rtp_keepalive_cmd, + "rtp keep-alive <1-120>", + RTP_STR RTP_KEEPALIVE_STR + "Keep-alive interval in secs\n" + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, atoi(argv[0])); + return CMD_SUCCESS; +} + +DEFUN(cfg_trunk_rtp_keepalive_once, + cfg_trunk_rtp_keepalive_once_cmd, + "rtp keep-alive once", + RTP_STR RTP_KEEPALIVE_STR + "Send dummy packet only once after CRCX/MDCX\n" + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, MGCP_KEEPALIVE_ONCE); + return CMD_SUCCESS; +} + +DEFUN(cfg_trunk_no_rtp_keepalive, + cfg_trunk_no_rtp_keepalive_cmd, + "no rtp keep-alive", + NO_STR RTP_STR RTP_KEEPALIVE_STR + ) +{ + struct mgcp_trunk_config *trunk = vty->index; + mgcp_trunk_set_keepalive(trunk, 0); + return CMD_SUCCESS; +} DEFUN(loop_endp, loop_endp_cmd, @@ -999,6 +1084,9 @@ int mgcp_vty_init(void) install_element(MGCP_NODE, &cfg_mgcp_rtp_transcoder_base_cmd); install_element(MGCP_NODE, &cfg_mgcp_rtp_ip_dscp_cmd); install_element(MGCP_NODE, &cfg_mgcp_rtp_ip_tos_cmd); + install_element(MGCP_NODE, &cfg_mgcp_rtp_keepalive_cmd); + install_element(MGCP_NODE, &cfg_mgcp_rtp_keepalive_once_cmd); + install_element(MGCP_NODE, &cfg_mgcp_no_rtp_keepalive_cmd); install_element(MGCP_NODE, &cfg_mgcp_agent_addr_cmd); install_element(MGCP_NODE, &cfg_mgcp_agent_addr_cmd_old); install_element(MGCP_NODE, &cfg_mgcp_transcoder_cmd); @@ -1024,6 +1112,9 @@ int mgcp_vty_init(void) install_element(MGCP_NODE, &cfg_mgcp_trunk_cmd); install_node(&trunk_node, config_write_trunk); vty_install_default(TRUNK_NODE); + install_element(TRUNK_NODE, &cfg_trunk_rtp_keepalive_cmd); + install_element(TRUNK_NODE, &cfg_trunk_rtp_keepalive_once_cmd); + install_element(TRUNK_NODE, &cfg_trunk_no_rtp_keepalive_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_number_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_name_cmd); install_element(TRUNK_NODE, &cfg_trunk_payload_number_cmd_old); diff --git a/openbsc/src/osmo-bsc_mgcp/mgcp_main.c b/openbsc/src/osmo-bsc_mgcp/mgcp_main.c index 596ea8a..14ec221 100644 --- a/openbsc/src/osmo-bsc_mgcp/mgcp_main.c +++ b/openbsc/src/osmo-bsc_mgcp/mgcp_main.c @@ -137,15 +137,6 @@ static int mgcp_rsip_cb(struct mgcp_trunk_config *tcfg) return 0; } -static int mgcp_change_cb(struct mgcp_trunk_config *cfg, int endpoint, int state) -{ - if (state != MGCP_ENDP_MDCX) - return 0; - - mgcp_send_dummy(&cfg->endpoints[endpoint]); - return 0; -} - static int read_call_agent(struct osmo_fd *fd, unsigned int what) { struct sockaddr_in addr; @@ -233,7 +224,6 @@ int main(int argc, char **argv) /* set some callbacks */ cfg->reset_cb = mgcp_rsip_cb; - cfg->change_cb = mgcp_change_cb; /* we need to bind a socket */ if (rc == 0) { diff --git a/openbsc/tests/mgcp/mgcp_test.ok b/openbsc/tests/mgcp/mgcp_test.ok index 6db7226..7e8aa5c 100644 --- a/openbsc/tests/mgcp/mgcp_test.ok +++ b/openbsc/tests/mgcp/mgcp_test.ok @@ -16,6 +16,7 @@ Testing AUEP2 Testing MDCX1 Testing MDCX2 Testing CRCX +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 1, BTS output enabled, NET output disabled @@ -24,18 +25,22 @@ Packet duration not set Requested packetization period not set Connection mode not set Testing MDCX4 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT1 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-40 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT2 +Dummy packets: 1 Detected packet duration: 40 Requested packetetization period: 20-20 Connection mode: 3, BTS output enabled, NET output enabled Testing MDCX4_PT3 +Dummy packets: 1 Detected packet duration: 40 Requested packetization period not set Connection mode: 3, BTS output enabled, NET output enabled @@ -48,6 +53,7 @@ Detected packet duration: 20 Requested packetization period not set Connection mode: 0, BTS output enabled, NET output enabled Testing CRCX_ZYN +Dummy packets: 1 Packet duration not set Requested packetization period not set Connection mode: 1, BTS output enabled, NET output disabled -- 1.7.9.5
participants (2)
-
Holger Hans Peter Freyther -
Jacob Erlbeck