pespin has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/osmo-mgw/+/39248?usp=email )


Change subject: mgw: Decouple SDP parsing step from conn obj update
......................................................................

mgw: Decouple SDP parsing step from conn obj update

This allows delaying modification of the conn before full parsing
succeeds.

Change-Id: I4a2aecb542886672c1f586c6607714e0356abe35
---
M include/osmocom/mgcp/mgcp_protocol.h
M include/osmocom/mgcp/mgcp_sdp.h
M src/libosmo-mgcp/mgcp_protocol.c
M src/libosmo-mgcp/mgcp_sdp.c
4 files changed, 118 insertions(+), 69 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/48/39248/1

diff --git a/include/osmocom/mgcp/mgcp_protocol.h 
b/include/osmocom/mgcp/mgcp_protocol.h
index ffe7ae0..a6a7cec 100644
--- a/include/osmocom/mgcp/mgcp_protocol.h
+++ b/include/osmocom/mgcp/mgcp_protocol.h
@@ -1,7 +1,35 @@
 #pragma once

+#include <stdint.h>
+#include <sys/socket.h>
+
 #include <osmocom/core/utils.h>
+#include <osmocom/core/socket.h>
 #include <osmocom/mgcp/mgcp_common.h>
+#include <osmocom/mgcp/mgcp_codec.h>
+
+
+#define MGCP_PARSE_SDP_PTIME_UNSET (-1)
+#define MGCP_PARSE_SDP_MAXPTIME_UNSET (-1)
+#define MGCP_PARSE_SDP_RTP_PORT_UNSET (0)
+
+struct mgcp_parse_sdp {
+       int ptime;
+       int maxptime;
+       int rtp_port;
+       struct osmo_sockaddr rem_addr; /* Only IP address, port is in rtp_port 
above */
+       struct mgcp_rtp_codecset cset;
+};
+
+static inline void mgcp_parse_sdp_init(struct mgcp_parse_sdp *sdp)
+{
+       sdp->ptime = MGCP_PARSE_SDP_PTIME_UNSET;
+       sdp->maxptime = MGCP_PARSE_SDP_MAXPTIME_UNSET;
+       sdp->rtp_port = MGCP_PARSE_SDP_RTP_PORT_UNSET;
+       sdp->rem_addr = (struct osmo_sockaddr){ .u.sa.sa_family = AF_UNSPEC };
+       mgcp_codecset_reset(&sdp->cset);
+}
+

 #define MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET (-2)
 #define MGCP_PARSE_HDR_PARS_OSMUX_CID_WILDCARD (-1)
@@ -39,6 +67,7 @@
        char *trans;
        /* MGCP Body: */
        struct mgcp_parse_hdr_pars hpars;
+       struct mgcp_parse_sdp sdp;
 };

 /* Local connection options */
diff --git a/include/osmocom/mgcp/mgcp_sdp.h b/include/osmocom/mgcp/mgcp_sdp.h
index 950092e..4a1313f 100644
--- a/include/osmocom/mgcp/mgcp_sdp.h
+++ b/include/osmocom/mgcp/mgcp_sdp.h
@@ -22,9 +22,9 @@

 #pragma once

-int mgcp_parse_sdp_data(const struct mgcp_endpoint *endp,
-                       struct mgcp_conn_rtp *conn,
-                       struct mgcp_parse_data *p);
+struct mgcp_parse_data;
+
+int mgcp_parse_sdp_data(struct mgcp_parse_data *p);

 int mgcp_write_response_sdp(const struct mgcp_endpoint *endp,
                            const struct mgcp_conn_rtp *conn, struct msgb *sdp,
diff --git a/src/libosmo-mgcp/mgcp_protocol.c b/src/libosmo-mgcp/mgcp_protocol.c
index d066f29..dfd3e0c 100644
--- a/src/libosmo-mgcp/mgcp_protocol.c
+++ b/src/libosmo-mgcp/mgcp_protocol.c
@@ -716,36 +716,59 @@
        return 0;
 }

+/* Apply parsed SDP information stored in struct mgcp_parse_sdp to conn_rtp: */
+static int handle_sdp(struct mgcp_conn_rtp *conn, struct mgcp_request_data *rq)
+{
+       OSMO_ASSERT(conn);
+       OSMO_ASSERT(rq);
+       struct mgcp_parse_data *p = rq->pdata;
+       OSMO_ASSERT(p);
+       OSMO_ASSERT(p->hpars.have_sdp);
+       struct mgcp_parse_sdp *sdp = &p->sdp;
+       struct mgcp_rtp_end *rtp;
+
+       rtp = &conn->end;
+
+       if (sdp->ptime != MGCP_PARSE_SDP_PTIME_UNSET)
+               mgcp_rtp_end_set_packet_duration_ms(rtp, sdp->ptime);
+
+       if (sdp->maxptime != MGCP_PARSE_SDP_MAXPTIME_UNSET)
+               rtp->maximum_packet_time = sdp->maxptime;
+
+       if (sdp->rem_addr.u.sa.sa_family != AF_UNSPEC) {
+               /* Keep port, only apply ip address: */
+               uint16_t port = osmo_sockaddr_port(&rtp->addr.u.sa);
+               memcpy(&rtp->addr, &sdp->rem_addr, sizeof(rtp->addr));
+               osmo_sockaddr_set_port(&rtp->addr.u.sa, port);
+       }
+
+       if (sdp->rtp_port != MGCP_PARSE_SDP_RTP_PORT_UNSET) {
+               osmo_sockaddr_set_port(&rtp->addr.u.sa, sdp->rtp_port);
+               rtp->rtcp_port = htons(sdp->rtp_port + 1);
+       }
+
+       /* Copy parsed codec set to conn: */
+       rtp->cset = sdp->cset;
+
+       return 0;
+}
+
 /* Process codec information contained in CRCX/MDCX */
-static int handle_codec_info(struct mgcp_conn_rtp *conn,
-                            struct mgcp_request_data *rq, int have_sdp, bool 
crcx)
+static int handle_codec_info(struct mgcp_conn_rtp *conn, struct 
mgcp_request_data *rq)
 {
        struct mgcp_endpoint *endp = rq->endp;
        struct mgcp_conn *conn_dst;
        struct mgcp_conn_rtp *conn_dst_rtp;
        struct mgcp_rtp_codecset *cset = &conn->end.cset;
-
        int rc;
-       char *cmd;
-
-       if (crcx)
-               cmd = "CRCX";
-       else
-               cmd = "MDCX";

        /* Collect codec information */
-       if (have_sdp) {
+       if (rq->pdata->hpars.have_sdp) {
                /* If we have SDP, we ignore the local connection options and
                 * use only the SDP information. */
-               mgcp_codecset_reset(cset);
-               rc = mgcp_parse_sdp_data(endp, conn, rq->pdata);
-               if (rc != 0) {
-                       LOGPCONN(conn->conn, DLMGCP,  LOGL_ERROR,
-                                "%s: sdp not parseable\n", cmd);
-
-                       /* See also RFC 3661: Protocol error */
-                       return 510;
-               }
+               rc = handle_sdp(conn, rq);
+               if (rc != 0)
+                       goto error;
        } else if (endp->local_options.codec) {
                /* When no SDP is available, we use the codec information from
                 * the local connection options (if present) */
@@ -780,9 +803,7 @@
        return 0;

 error:
-       LOGPCONN(conn->conn, DLMGCP, LOGL_ERROR,
-            "%s: codec negotiation failure\n", cmd);
-
+       LOGPCONN(conn->conn, DLMGCP, LOGL_ERROR, "%s: codec negotiation 
failure\n", rq->name);
        /* See also RFC 3661: Codec negotiation failure */
        return 534;
 }
@@ -861,6 +882,15 @@
                return create_err_response(endp, endp, 523, "CRCX", 
pdata->trans);
        }

+       /* Parse SDP if found: */
+       if (hpars->have_sdp) {
+               rc = mgcp_parse_sdp_data(pdata);
+               if (rc < 0) { /* See also RFC 3661: Protocol error */
+                       LOGPENDP(endp, DLMGCP,  LOGL_ERROR, "CRCX: sdp not 
parseable\n");
+                       return create_err_response(endp, endp, 510, "CRCX", 
pdata->trans);
+               }
+       }
+
        /* If osmux is disabled, just skip setting it up */
        if (trunk->cfg->osmux.usage == OSMUX_USAGE_OFF)
                hpars->remote_osmux_cid = MGCP_PARSE_HDR_PARS_OSMUX_CID_UNSET;
@@ -969,7 +999,7 @@
        }

        /* Handle codec information and decide for a suitable codec */
-       rc = handle_codec_info(conn_rtp, rq, hpars->have_sdp, true);
+       rc = handle_codec_info(conn_rtp, rq);
        mgcp_codecset_summary(&conn_rtp->end.cset, mgcp_conn_dump(conn));
        if (rc) {
                error_code = rc;
@@ -1103,6 +1133,15 @@
                return create_err_response(endp, endp, error_code, "MDCX", 
pdata->trans);
        }

+       /* Parse SDP if found: */
+       if (hpars->have_sdp) {
+               rc = mgcp_parse_sdp_data(pdata);
+               if (rc < 0) { /* See also RFC 3661: Protocol error */
+                       LOGPENDP(endp, DLMGCP,  LOGL_ERROR, "MDCX: sdp not 
parseable\n");
+                       return create_err_response(endp, endp, 510, "MDCX", 
pdata->trans);
+               }
+       }
+
        conn = mgcp_endp_get_conn(endp, hpars->connid);
        if (!conn) {
                rate_ctr_inc(rate_ctr_group_get_ctr(rate_ctrs, 
MGCP_MDCX_FAIL_CONN_NOT_FOUND));
@@ -1136,7 +1175,7 @@
        OSMO_ASSERT(conn_rtp);

        /* Handle codec information and decide for a suitable codec */
-       rc = handle_codec_info(conn_rtp, rq, hpars->have_sdp, false);
+       rc = handle_codec_info(conn_rtp, rq);
        mgcp_codecset_summary(&conn_rtp->end.cset, mgcp_conn_dump(conn));
        if (rc) {
                error_code = rc;
diff --git a/src/libosmo-mgcp/mgcp_sdp.c b/src/libosmo-mgcp/mgcp_sdp.c
index ffd0f18..1d76ddb 100644
--- a/src/libosmo-mgcp/mgcp_sdp.c
+++ b/src/libosmo-mgcp/mgcp_sdp.c
@@ -312,16 +312,13 @@
 }

 /*! Analyze SDP input string.
- *  \param[in] endp trunk endpoint.
- *  \param[out] conn associated rtp connection.
- *  \param[out] caller provided memory to store the parsing results.
+ *  \param[inout] p provided memory to store the parsing results.
  *
- *  Note: In conn (conn->end) the function returns the packet duration,
- *  rtp port, rtcp port and the codec information.
  *  \returns 0 on success, -1 on failure. */
-int mgcp_parse_sdp_data(const struct mgcp_endpoint *endp,
-                       struct mgcp_conn_rtp *conn, struct mgcp_parse_data *p)
+int mgcp_parse_sdp_data(struct mgcp_parse_data *p)
 {
+       OSMO_ASSERT(p);
+       struct mgcp_parse_sdp *sdp = &p->sdp;
        struct sdp_rtp_map codecs[MGCP_MAX_CODECS];
        unsigned int codecs_used = 0;
        struct sdp_fmtp_param fmtp_params[MGCP_MAX_CODECS];
@@ -331,19 +328,14 @@
        char *line;
        unsigned int i;
        void *tmp_ctx = talloc_new(NULL);
-       struct mgcp_rtp_end *rtp;

        int payload_type;
        int ptime, ptime2 = 0;
        char audio_name[64];
        int port, rc;

-       OSMO_ASSERT(endp);
-       OSMO_ASSERT(conn);
-       OSMO_ASSERT(p);
-
-       rtp = &conn->end;
        memset(&codecs, 0, sizeof(codecs));
+       mgcp_parse_sdp_init(sdp);

        for_each_line(line, p->save) {
                switch (line[0]) {
@@ -361,19 +353,19 @@

                        if (sscanf(line, "a=ptime:%d-%d", &ptime, &ptime2) >= 
1) {
                                if (ptime2 > 0 && ptime2 != ptime)
-                                       
mgcp_rtp_end_set_packet_duration_ms(rtp, 0);
+                                       sdp->ptime = 0;
                                else
-                                       
mgcp_rtp_end_set_packet_duration_ms(rtp, ptime);
+                                       sdp->ptime = ptime;
                                break;
                        }

                        if (sscanf(line, "a=maxptime:%d", &ptime2) == 1) {
-                               rtp->maximum_packet_time = ptime2;
+                               sdp->maxptime = ptime2;
                                break;
                        }

                        if (strncmp("a=fmtp:", line, 6) == 0) {
-                               rc = fmtp_from_sdp(conn->conn, 
&fmtp_params[fmtp_used], line);
+                               rc = fmtp_from_sdp(tmp_ctx, 
&fmtp_params[fmtp_used], line);
                                if (rc >= 0)
                                        fmtp_used++;
                                break;
@@ -382,33 +374,23 @@
                        break;
                case 'm':
                        rc = sscanf(line, "m=audio %d RTP/AVP", &port);
-                       if (rc == 1) {
-                               osmo_sockaddr_set_port(&rtp->addr.u.sa, port);
-                               rtp->rtcp_port = htons(port + 1);
-                       }
+                       if (rc == 1)
+                               sdp->rtp_port = port;

-                       rc = pt_from_sdp(conn->conn, codecs,
-                                        ARRAY_SIZE(codecs), line);
+                       rc = pt_from_sdp(tmp_ctx, codecs, ARRAY_SIZE(codecs), 
line);
                        if (rc > 0)
                                codecs_used = rc;
                        break;
                case 'c':
-                       if (audio_ip_from_sdp(&rtp->addr, line) < 0) {
+                       if (audio_ip_from_sdp(&sdp->rem_addr, line) < 0) {
                                talloc_free(tmp_ctx);
                                return -1;
                        }
                        break;
                default:
-                       if (endp)
-                               /* TODO: Check spec: We used the bare endpoint 
number before,
-                                * now we use the endpoint name as a whole? Is 
this allowed? */
-                               LOGP(DLMGCP, LOGL_NOTICE,
-                                    "Unhandled SDP option: '%c'/%d on %s\n",
-                                    line[0], line[0], endp->name);
-                       else
-                               LOGP(DLMGCP, LOGL_NOTICE,
-                                    "Unhandled SDP option: '%c'/%d\n",
-                                    line[0], line[0]);
+                       LOGP(DLMGCP, LOGL_NOTICE,
+                            "Unhandled SDP option: '%c'/%d on %s\n",
+                            line[0], line[0], p->epname);
                        break;
                }
        }
@@ -422,23 +404,22 @@
        /* Store parsed codec information */
        for (i = 0; i < codecs_used; i++) {
                codec_param = param_by_pt(codecs[i].payload_type, fmtp_params, 
fmtp_used);
-               rc = mgcp_codecset_add_codec(&conn->end.cset, 
codecs[i].payload_type, codecs[i].map_line, codec_param);
+               rc = mgcp_codecset_add_codec(&sdp->cset, 
codecs[i].payload_type, codecs[i].map_line, codec_param);
                if (rc < 0)
-                       LOGPENDP(endp, DLMGCP, LOGL_NOTICE, "failed to add 
codec\n");
+                       LOGP(DLMGCP, LOGL_NOTICE, "%s: failed to add codec\n", 
p->epname);
        }

        talloc_free(tmp_ctx);

-       LOGPCONN(conn->conn, DLMGCP, LOGL_NOTICE,
-            "Got media info via SDP: port:%d, addr:%s, duration:%d, 
payload-types:",
-            osmo_sockaddr_port(&rtp->addr.u.sa), 
osmo_sockaddr_ntop(&rtp->addr.u.sa, ipbuf),
-            rtp->packet_duration_ms);
+       LOGP(DLMGCP, LOGL_NOTICE,
+            "%s: Got media info via SDP: port:%d, addr:%s, duration:%d, 
payload-types:",
+            p->epname, sdp->rtp_port, osmo_sockaddr_ntop(&sdp->rem_addr.u.sa, 
ipbuf), sdp->ptime);
        if (codecs_used == 0)
                LOGPC(DLMGCP, LOGL_NOTICE, "none");
        for (i = 0; i < codecs_used; i++) {
                LOGPC(DLMGCP, LOGL_NOTICE, "%d=%s",
-                     rtp->cset.codecs[i].payload_type,
-                     strlen(rtp->cset.codecs[i].subtype_name) ? 
rtp->cset.codecs[i].subtype_name : "unknown");
+                     sdp->cset.codecs[i].payload_type,
+                     strlen(sdp->cset.codecs[i].subtype_name) ? 
sdp->cset.codecs[i].subtype_name : "unknown");
                LOGPC(DLMGCP, LOGL_NOTICE, " ");
        }
        LOGPC(DLMGCP, LOGL_NOTICE, "\n");

--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/39248?usp=email
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings?usp=email

Gerrit-MessageType: newchange
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I4a2aecb542886672c1f586c6607714e0356abe35
Gerrit-Change-Number: 39248
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <[email protected]>

Reply via email to