[PATCH 1/2] ipa/test: Add test program for IPA message reception
This patch adds tests for ipa_msg_recv(), where messages are sent either complete or partitioned. Sponsored-by: On-Waves ehf --- tests/Makefile.am | 11 +- tests/ipa_recv/ipa_recv_test.c | 247 +++++++++++++++++++++++++++++++++++++++ tests/ipa_recv/ipa_recv_test.ok | 12 ++ tests/testsuite.at | 7 ++ 4 files changed, 275 insertions(+), 2 deletions(-) create mode 100644 tests/ipa_recv/ipa_recv_test.c create mode 100644 tests/ipa_recv/ipa_recv_test.ok diff --git a/tests/Makefile.am b/tests/Makefile.am index af8a9cf..c8b8996 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -5,7 +5,8 @@ AM_LDFLAGS = $(COVERAGE_LDFLAGS) check_PROGRAMS = e1inp_ipa_bsc_test \ e1inp_ipa_bts_test \ ipa_proxy_test \ - subchan_demux/subchan_demux_test + subchan_demux/subchan_demux_test \ + ipa_recv/ipa_recv_test e1inp_ipa_bsc_test_SOURCES = e1inp_ipa_bsc_test.c e1inp_ipa_bsc_test_LDADD = $(top_builddir)/src/libosmoabis.la \ @@ -25,6 +26,11 @@ subchan_demux_subchan_demux_test_LDADD = $(top_builddir)/src/libosmoabis.la \ $(LIBOSMOCORE_LIBS) $(LIBOSMOGSM_LIBS) \ $(LIBOSMOVTY_LIBS) +ipa_recv_ipa_recv_test_SOURCES = ipa_recv/ipa_recv_test.c +ipa_recv_ipa_recv_test_LDADD = $(top_builddir)/src/libosmoabis.la \ + $(LIBOSMOCORE_LIBS) $(LIBOSMOGSM_LIBS) \ + $(LIBOSMOVTY_LIBS) + # boilerplate for the tests # The `:;' works around a Bash 3.2 bug when the output is not writeable. $(srcdir)/package.m4: $(top_srcdir)/configure.ac @@ -45,7 +51,8 @@ $(srcdir)/package.m4: $(top_srcdir)/configure.ac } >'$(srcdir)/package.m4' EXTRA_DIST = testsuite.at $(srcdir)/package.m4 $(TESTSUITE) \ - subchan_demux/subchan_demux_test.ok + subchan_demux/subchan_demux_test.ok \ + ipa_recv/ipa_recv_test.ok TESTSUITE = $(srcdir)/testsuite diff --git a/tests/ipa_recv/ipa_recv_test.c b/tests/ipa_recv/ipa_recv_test.c new file mode 100644 index 0000000..7b26259 --- /dev/null +++ b/tests/ipa_recv/ipa_recv_test.c @@ -0,0 +1,247 @@ +/* IPA receive test */ + +/* (C) 2014 by On-Waves + * + * All Rights Reserved + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * 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/>. + * + */ + +#include <osmocom/abis/e1_input.h> + +#include <osmocom/abis/ipa.h> +#include <osmocom/core/utils.h> +#include <osmocom/core/msgb.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/application.h> + +#include <stdio.h> +#include <string.h> +#include <sys/socket.h> +#include <sys/types.h> +#include <unistd.h> +#include <fcntl.h> +#include <errno.h> +#include <err.h> + +#if 0 +/* IPA protocol ip.access, type: RSL */ +static const unsigned char gsm_ipa_chanrqd[13] = { + 0x00, 0x0a, 0x00, 0x0c, 0x13, 0x01, 0x88, 0x13, + 0x4e, 0x42, 0x93, 0x11, 0x00 +}; + +/* IPA protocol ip.access, type: RSL */ +static const unsigned char gsm_ipa_chanactivack[10] = { + 0x00, 0x07, 0x00, 0x08, 0x22, 0x01, 0x12, 0x08, + 0x43, 0x81 +}; + +/* IPA protocol ip.access, type: RSL */ +static const unsigned char gsm_ipa_data[40] = { + 0x00, 0x25, 0x00, 0x08, 0x28, 0x01, 0x12, 0x1b, + 0x00, 0x19, 0x03, 0x3b, 0x3b, 0x00, 0x04, 0x00, + 0x0a, 0x38, 0x00, 0x0b, 0x00, 0x12, 0x06, 0x15, + 0x35, 0x40, 0x7e, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 +}; +#endif + +static const char *ipa_test_messages[] = { + "Hello IPA", + "A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz", + "Hello again IPA", + "", + "Next is empty", + NULL, + "Bye", + "Bye", +}; + +static void append_ipa_message(struct msgb *msg, int proto, const char *text) +{ + int len = 0; + unsigned char *l2; + + if (text) + len = strlen(text) + 1; + + msgb_put_u16(msg, len); + msgb_put_u8(msg, proto); + + l2 = msgb_put(msg, len); + if (text) + strcpy((char *)l2, text); +} + +static int receive_messages(int fd) +{ + struct msgb *msg; + char dummy; + int rc; + while (1) { + if (recv(fd, &dummy, 1, MSG_PEEK) < 1) { + rc = -EAGAIN; + break; + } + msg = NULL; + rc = ipa_msg_recv(fd, &msg); + if (rc == -1) + rc = -errno; + fprintf(stderr, + "ipa_msg_recv: %d, msg %s NULL\n", + rc, msg ? "!=" : "=="); + if (rc == -EAGAIN) + printf( "got msg %s NULL, " + "returned: %s\n", + msg ? "!=" : "==", + rc == 0 ? "EOF" : + rc > 0 ? "OK" : + strerror(-rc)); + if (rc == 0) + return 0; + if (rc == -EAGAIN) + break; + if (rc < 0) { + printf("ipa_msg_recv failed with: %s\n", strerror(-rc)); + return rc; + } + printf("got IPA message, size=%d, proto=%d, text=\"%s\"\n", + rc, msg->data[2], msg->l2h); + msgb_free(msg); + }; + + return rc; +} + +static int slurp_data(int fd) { + int rc; + char buf[256]; + int count = 0; + + do { + rc = recv(fd, buf, sizeof(buf), 0); + if (rc <= 0) + break; + + count += rc; + } while (1); + + return count; +}; + +static void test_complete_recv(void) +{ + int sv[2]; + struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + int rc, i; + + printf("Testing IPA recv with complete messages.\n"); + + if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) + err(1, "socketpair"); + + fcntl(sv[0], F_SETFL, O_NONBLOCK); + + for (i=0; i < ARRAY_SIZE(ipa_test_messages); i++) + append_ipa_message(msg_out, 200, ipa_test_messages[i]); + + while (msg_out->len > 0) { + rc = write(sv[1], msg_out->data, msg_out->len); + if (rc == -1) + err(1, "write"); + msgb_pull(msg_out, rc); + } + + for (i=0; i < ARRAY_SIZE(ipa_test_messages); i++) { + rc = receive_messages(sv[0]); + if (rc == 0) + break; + + if (rc < 0 && rc != -EAGAIN) + break; + } + + rc = slurp_data(sv[0]); + printf("done: unread %d, unsent %d\n", rc, msg_out->len); + + close(sv[1]); + close(sv[0]); + + msgb_free(msg_out); +} + + +static void test_partial_recv(void) +{ + int sv[2]; + struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + int rc, i; + + printf("Testing IPA recv with partitioned messages.\n"); + + if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) + err(1, "socketpair"); + + fcntl(sv[0], F_SETFL, O_NONBLOCK); + + for (i=0; i < ARRAY_SIZE(ipa_test_messages); i++) + append_ipa_message(msg_out, 200, ipa_test_messages[i]); + + while (msg_out->len > 0) { + int len = 5; + if (len > msg_out->len) + len = msg_out->len; + if (write(sv[1], msg_out->data, len) == -1) + err(1, "write"); + msgb_pull(msg_out, len); + + if (msg_out->len == 0) + shutdown(sv[1], SHUT_WR); + + rc = receive_messages(sv[0]); + + if (rc == 0) + break; + + if (rc < 0 && rc != -EAGAIN) + break; + } + rc = slurp_data(sv[0]); + printf("done: unread %d, unsent %d\n", rc, msg_out->len); + + close(sv[1]); + close(sv[0]); + + msgb_free(msg_out); +} + +static struct log_info info = {}; + +int main(int argc, char **argv) +{ + osmo_init_logging(&info); + log_set_all_filter(osmo_stderr_target, 1); + log_set_log_level(osmo_stderr_target, LOGL_INFO); + + printf("Testing the IPA layer.\n"); + + /* run the tests */ + test_complete_recv(); + test_partial_recv(); + + printf("No crashes.\n"); + return 0; +} diff --git a/tests/ipa_recv/ipa_recv_test.ok b/tests/ipa_recv/ipa_recv_test.ok new file mode 100644 index 0000000..4144d47 --- /dev/null +++ b/tests/ipa_recv/ipa_recv_test.ok @@ -0,0 +1,12 @@ +Testing the IPA layer. +Testing IPA recv with complete messages. +got IPA message, size=10, proto=200, text="Hello IPA" +got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz" +got IPA message, size=16, proto=200, text="Hello again IPA" +got IPA message, size=1, proto=200, text="" +got IPA message, size=14, proto=200, text="Next is empty" +done: unread 14, unsent 0 +Testing IPA recv with partitioned messages. +ipa_msg_recv failed with: Input/output error +done: unread 0, unsent 154 +No crashes. diff --git a/tests/testsuite.at b/tests/testsuite.at index 04193c5..ff550b0 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -11,8 +11,15 @@ AT_BANNER([Regression tests.]) # AT_CHECK([$abs_top_builddir/tests/NAME/NAME_test], [], [expout]) # AT_CLEANUP +AT_SETUP([ipa_recv]) +AT_KEYWORDS([ipa_recv]) +cat $abs_srcdir/ipa_recv/ipa_recv_test.ok > expout +AT_CHECK([$abs_top_builddir/tests/ipa_recv/ipa_recv_test], [], [expout],[ignore]) +AT_CLEANUP + AT_SETUP([subchan_demux]) AT_KEYWORDS([subchan_demux]) cat $abs_srcdir/subchan_demux/subchan_demux_test.ok > expout AT_CHECK([$abs_top_builddir/tests/subchan_demux/subchan_demux_test], [], [expout]) AT_CLEANUP + -- 1.7.9.5
Currently ipa_msg_recv() fails, when messages are received partially. This patch provides a new function ipa_msg_recv_buffered() that uses an additional ** to a message buffer to store partial data. When this happens, -EAGAIN is returned. If NULL is used, the function behaves similar to ipa_msg_recv() and fails on partial read. In addition iin case of errors the return value is now always -EXXX and the contents of errno is undefined. Note that this feature needs support by the calling code insofar that *tmp_msg must be set to NULL initially and it must be freed and set to NULL manually when the socket is closed. Note also that ipa_msg_recv() is then a wrapper around ipa_msg_recv_buffered() which mimics the old error behaviour by setting errno explicitely to -rc and returning -1 when an error has happened. Ticket: OW#728 Sponsored-by: On-Waves ehf --- include/osmocom/abis/e1_input.h | 2 + include/osmocom/abis/ipa.h | 3 + src/input/ipa.c | 151 ++++++++++++++++++++++++++++++--------- src/input/ipaccess.c | 13 +++- tests/ipa_recv/ipa_recv_test.c | 55 +++++++++----- tests/ipa_recv/ipa_recv_test.ok | 27 ++++++- 6 files changed, 197 insertions(+), 54 deletions(-) diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h index 9b77893..cf8677b 100644 --- a/include/osmocom/abis/e1_input.h +++ b/include/osmocom/abis/e1_input.h @@ -109,6 +109,8 @@ struct e1inp_ts { struct osmo_fd fd; } rs232; } driver; + + struct msgb *pending_msg; }; struct gsm_e1_subslot { diff --git a/include/osmocom/abis/ipa.h b/include/osmocom/abis/ipa.h index d577d74..982b694 100644 --- a/include/osmocom/abis/ipa.h +++ b/include/osmocom/abis/ipa.h @@ -27,6 +27,7 @@ struct ipa_server_conn { int (*closed_cb)(struct ipa_server_conn *peer); int (*cb)(struct ipa_server_conn *peer, struct msgb *msg); void *data; + struct msgb *pending_msg; }; struct ipa_server_conn *ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, int (*cb)(struct ipa_server_conn *peer, struct msgb *msg), int (*closed_cb)(struct ipa_server_conn *peer), void *data); @@ -53,6 +54,7 @@ struct ipa_client_conn { int (*read_cb)(struct ipa_client_conn *link, struct msgb *msg); int (*write_cb)(struct ipa_client_conn *link); void *data; + struct msgb *pending_msg; }; struct ipa_client_conn *ipa_client_conn_create(void *ctx, struct e1inp_ts *ts, int priv_nr, const char *addr, uint16_t port, void (*updown)(struct ipa_client_conn *link, int), int (*read_cb)(struct ipa_client_conn *link, struct msgb *msgb), int (*write_cb)(struct ipa_client_conn *link), void *data); @@ -64,6 +66,7 @@ void ipa_client_conn_close(struct ipa_client_conn *link); void ipa_client_conn_send(struct ipa_client_conn *link, struct msgb *msg); int ipa_msg_recv(int fd, struct msgb **rmsg); +int ipa_msg_recv_buffered(int fd, struct msgb **rmsg, struct msgb **tmp_msg); int ipaccess_rcvmsg_base(struct msgb *msg, struct osmo_fd *bfd); diff --git a/src/input/ipa.c b/src/input/ipa.c index b5abd36..71e1227 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -49,50 +49,130 @@ void ipa_msg_push_header(struct msgb *msg, uint8_t proto) int ipa_msg_recv(int fd, struct msgb **rmsg) { - struct msgb *msg; + int rc = ipa_msg_recv_buffered(fd, rmsg, NULL); + if (rc < 0) { + errno = -rc; + rc = -1; + } + return rc; +} + +int ipa_msg_recv_buffered(int fd, struct msgb **rmsg, struct msgb **tmp_msg) +{ + struct msgb *msg = tmp_msg ? *tmp_msg : NULL; struct ipaccess_head *hh; int len, ret; + int needed; - msg = ipa_msg_alloc(0); if (msg == NULL) - return -ENOMEM; + msg = ipa_msg_alloc(0); - /* first read our 3-byte header */ - hh = (struct ipaccess_head *) msg->data; - ret = recv(fd, msg->data, sizeof(*hh), 0); - if (ret <= 0) { - msgb_free(msg); - return ret; - } else if (ret != sizeof(*hh)) { - LOGP(DLINP, LOGL_ERROR, "too small message received\n"); - msgb_free(msg); - return -EIO; + if (msg == NULL) { + ret = -ENOMEM; + goto discard_msg; } - msgb_put(msg, ret); + + if (msg->l2h == NULL) { + /* first read our 3-byte header */ + needed = sizeof(*hh) - msg->len; + ret = recv(fd, msg->tail, needed, 0); + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + if (msg->len == 0) { + ret = -EAGAIN; + goto discard_msg; + } + + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message header (%d/%d)\n", + msg->len, sizeof(*hh)); + if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } + + msg->l2h = msg->tail; + } + + hh = (struct ipaccess_head *) msg->data; /* then read the length as specified in header */ - msg->l2h = msg->data + sizeof(*hh); len = ntohs(hh->len); if (len < 0 || IPA_ALLOC_SIZE < len + sizeof(*hh)) { LOGP(DLINP, LOGL_ERROR, "bad message length of %d bytes, " - "received %d bytes\n", len, ret); - msgb_free(msg); - return -EIO; + "received %d bytes\n", len, msg->len); + ret = -EIO; + goto discard_msg; } - ret = recv(fd, msg->l2h, len, 0); - if (ret <= 0) { - msgb_free(msg); - return ret; - } else if (ret < len) { - LOGP(DLINP, LOGL_ERROR, "truncated message received\n"); - msgb_free(msg); - return -EIO; + needed = len - msgb_l2len(msg); + + if (needed > 0) { + ret = recv(fd, msg->tail, needed, 0); + + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message L2 data (%d/%d)\n", + msgb_l2len(msg), len); + if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } } - msgb_put(msg, ret); + + ret = msgb_l2len(msg); + + if (ret == 0) { + LOGP(DLINP, LOGL_INFO, + "Discarding IPA message without payload\n"); + ret = -EAGAIN; + goto discard_msg; + } + + if (tmp_msg) + *tmp_msg = NULL; *rmsg = msg; return ret; + +discard_msg: + if (tmp_msg) + *tmp_msg = NULL; + msgb_free(msg); + return ret; } void ipa_client_conn_close(struct ipa_client_conn *link) @@ -103,6 +183,8 @@ void ipa_client_conn_close(struct ipa_client_conn *link) close(link->ofd->fd); link->ofd->fd = -1; } + msgb_free(link->pending_msg); + link->pending_msg = NULL; } static void ipa_client_read(struct ipa_client_conn *link) @@ -113,11 +195,12 @@ static void ipa_client_read(struct ipa_client_conn *link) LOGP(DLINP, LOGL_DEBUG, "message received\n"); - ret = ipa_msg_recv(ofd->fd, &msg); + ret = ipa_msg_recv_buffered(ofd->fd, &msg, &link->pending_msg); if (ret < 0) { - if (errno == EPIPE || errno == ECONNRESET) { + if (ret == -EAGAIN) + return; + if (ret == -EPIPE || ret == -ECONNRESET) LOGP(DLINP, LOGL_ERROR, "lost connection with server\n"); - } ipa_client_conn_close(link); if (link->updown_cb) link->updown_cb(link, 0); @@ -382,11 +465,12 @@ static void ipa_server_conn_read(struct ipa_server_conn *conn) LOGP(DLINP, LOGL_DEBUG, "message received\n"); - ret = ipa_msg_recv(ofd->fd, &msg); + ret = ipa_msg_recv_buffered(ofd->fd, &msg, &conn->pending_msg); if (ret < 0) { - if (errno == EPIPE || errno == ECONNRESET) { + if (ret == -EAGAIN) + return; + if (ret == -EPIPE || ret == -ECONNRESET) LOGP(DLINP, LOGL_ERROR, "lost connection with server\n"); - } ipa_server_conn_destroy(conn); return; } else if (ret == 0) { @@ -471,6 +555,7 @@ ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, void ipa_server_conn_destroy(struct ipa_server_conn *conn) { close(conn->ofd.fd); + msgb_free(conn->pending_msg); osmo_fd_unregister(&conn->ofd); if (conn->closed_cb) conn->closed_cb(conn); diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 225d70c..92ad619 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -258,6 +258,8 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, static int ipaccess_drop(struct osmo_fd *bfd, struct e1inp_line *line) { int ret = 1; + unsigned int ts_nr = bfd->priv_nr; + struct e1inp_ts *e1i_ts = &line->ts[ts_nr-1]; /* Error case: we did not see any ID_RESP yet for this socket. */ if (bfd->fd != -1) { @@ -269,6 +271,9 @@ static int ipaccess_drop(struct osmo_fd *bfd, struct e1inp_line *line) ret = -ENOENT; } + msgb_free(e1i_ts->pending_msg); + e1i_ts->pending_msg = NULL; + /* e1inp_sign_link_destroy releases the socket descriptors for us. */ line->ops->sign_link_down(line); @@ -415,13 +420,15 @@ static int handle_ts1_read(struct osmo_fd *bfd) struct e1inp_ts *e1i_ts = &line->ts[ts_nr-1]; struct e1inp_sign_link *link; struct ipaccess_head *hh; - struct msgb *msg; + struct msgb *msg = NULL; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &e1i_ts->pending_msg); if (ret < 0) { + if (ret == -EAGAIN) + return; LOGP(DLINP, LOGL_NOTICE, "Sign link problems, " - "closing socket. Reason: %s\n", strerror(errno)); + "closing socket. Reason: %s\n", strerror(-ret)); goto err; } else if (ret == 0) { LOGP(DLINP, LOGL_NOTICE, "Sign link vanished, dead socket\n"); diff --git a/tests/ipa_recv/ipa_recv_test.c b/tests/ipa_recv/ipa_recv_test.c index 7b26259..8cdc7e2 100644 --- a/tests/ipa_recv/ipa_recv_test.c +++ b/tests/ipa_recv/ipa_recv_test.c @@ -86,7 +86,7 @@ static void append_ipa_message(struct msgb *msg, int proto, const char *text) strcpy((char *)l2, text); } -static int receive_messages(int fd) +static int receive_messages(int fd, struct msgb **pending_msg) { struct msgb *msg; char dummy; @@ -97,13 +97,22 @@ static int receive_messages(int fd) break; } msg = NULL; - rc = ipa_msg_recv(fd, &msg); - if (rc == -1) - rc = -errno; + rc = ipa_msg_recv_buffered(fd, &msg, pending_msg); + fprintf(stderr, - "ipa_msg_recv: %d, msg %s NULL\n", - rc, msg ? "!=" : "=="); - if (rc == -EAGAIN) + "ipa_msg_recv_buffered: %d, msg %s NULL, " + "pending_msg %s NULL\n", + rc, msg ? "!=" : "==", + !pending_msg ? "??" : *pending_msg ? "!=" : "=="); + if (pending_msg && !!msg == !!*pending_msg) + printf( "got msg %s NULL, pending_msg %s NULL, " + "returned: %s\n", + msg ? "!=" : "==", + *pending_msg ? "!=" : "==", + rc == 0 ? "EOF" : + rc > 0 ? "OK" : + strerror(-rc)); + else if (!pending_msg && rc == -EAGAIN) printf( "got msg %s NULL, " "returned: %s\n", msg ? "!=" : "==", @@ -115,7 +124,8 @@ static int receive_messages(int fd) if (rc == -EAGAIN) break; if (rc < 0) { - printf("ipa_msg_recv failed with: %s\n", strerror(-rc)); + printf("ipa_msg_recv_buffered failed with: %s\n", + strerror(-rc)); return rc; } printf("got IPA message, size=%d, proto=%d, text=\"%s\"\n", @@ -142,13 +152,15 @@ static int slurp_data(int fd) { return count; }; -static void test_complete_recv(void) +static void test_complete_recv(int do_not_assemble) { int sv[2]; struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + struct msgb *pending_msg = NULL; int rc, i; - printf("Testing IPA recv with complete messages.\n"); + printf("Testing IPA recv with complete messages%s.\n", + do_not_assemble ? "" : " with assembling enabled"); if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) err(1, "socketpair"); @@ -166,7 +178,11 @@ static void test_complete_recv(void) } for (i=0; i < ARRAY_SIZE(ipa_test_messages); i++) { - rc = receive_messages(sv[0]); + rc = receive_messages(sv[0], + do_not_assemble ? NULL : &pending_msg); + if (pending_msg) + printf("Unexpected partial message: size=%d\n", + pending_msg->len); if (rc == 0) break; @@ -181,16 +197,19 @@ static void test_complete_recv(void) close(sv[0]); msgb_free(msg_out); + msgb_free(pending_msg); } -static void test_partial_recv(void) +static void test_partial_recv(int do_not_assemble) { int sv[2]; struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + struct msgb *pending_msg = NULL; int rc, i; - printf("Testing IPA recv with partitioned messages.\n"); + printf("Testing IPA recv with partitioned messages%s.\n", + do_not_assemble ? "" : " with assembling enabled"); if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) err(1, "socketpair"); @@ -211,7 +230,8 @@ static void test_partial_recv(void) if (msg_out->len == 0) shutdown(sv[1], SHUT_WR); - rc = receive_messages(sv[0]); + rc = receive_messages(sv[0], + do_not_assemble ? NULL : &pending_msg); if (rc == 0) break; @@ -226,6 +246,7 @@ static void test_partial_recv(void) close(sv[0]); msgb_free(msg_out); + msgb_free(pending_msg); } static struct log_info info = {}; @@ -239,8 +260,10 @@ int main(int argc, char **argv) printf("Testing the IPA layer.\n"); /* run the tests */ - test_complete_recv(); - test_partial_recv(); + test_complete_recv(1); + test_partial_recv(1); + test_complete_recv(0); + test_partial_recv(0); printf("No crashes.\n"); return 0; diff --git a/tests/ipa_recv/ipa_recv_test.ok b/tests/ipa_recv/ipa_recv_test.ok index 4144d47..bdbfb7d 100644 --- a/tests/ipa_recv/ipa_recv_test.ok +++ b/tests/ipa_recv/ipa_recv_test.ok @@ -5,8 +5,31 @@ got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMN got IPA message, size=16, proto=200, text="Hello again IPA" got IPA message, size=1, proto=200, text="" got IPA message, size=14, proto=200, text="Next is empty" -done: unread 14, unsent 0 +got msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 Testing IPA recv with partitioned messages. -ipa_msg_recv failed with: Input/output error +ipa_msg_recv_buffered failed with: Input/output error done: unread 0, unsent 154 +Testing IPA recv with complete messages with assembling enabled. +got IPA message, size=10, proto=200, text="Hello IPA" +got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz" +got IPA message, size=16, proto=200, text="Hello again IPA" +got IPA message, size=1, proto=200, text="" +got IPA message, size=14, proto=200, text="Next is empty" +got msg == NULL, pending_msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 +Testing IPA recv with partitioned messages with assembling enabled. +got IPA message, size=10, proto=200, text="Hello IPA" +got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz" +got IPA message, size=16, proto=200, text="Hello again IPA" +got IPA message, size=1, proto=200, text="" +got IPA message, size=14, proto=200, text="Next is empty" +got msg == NULL, pending_msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 No crashes. -- 1.7.9.5
The old ipa_msg_recv() implementation didn't support partial receive, so IPA connections got disconnected when this happened. This patch adds the handling of the temporary message buffers and uses ipa_msg_recv_buffered(). It has been successfully tested by jerlbeck with osmo-nitb and osmo-bsc. Ticket: OW#768 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/bsc_msc.h | 2 ++ openbsc/include/openbsc/bsc_nat.h | 5 +++++ openbsc/include/openbsc/control_cmd.h | 3 +++ openbsc/src/libbsc/bsc_msc.c | 10 ++++++++++ openbsc/src/libctrl/control_if.c | 5 ++++- openbsc/src/osmo-bsc/osmo_bsc_msc.c | 6 ++++-- openbsc/src/osmo-bsc_nat/bsc_nat.c | 19 +++++++++++++++---- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 8 ++++++-- 8 files changed, 49 insertions(+), 9 deletions(-) diff --git a/openbsc/include/openbsc/bsc_msc.h b/openbsc/include/openbsc/bsc_msc.h index 647f47e..0adbd26 100644 --- a/openbsc/include/openbsc/bsc_msc.h +++ b/openbsc/include/openbsc/bsc_msc.h @@ -48,6 +48,8 @@ struct bsc_msc_connection { void (*connected) (struct bsc_msc_connection *); struct osmo_timer_list reconnect_timer; struct osmo_timer_list timeout_timer; + + struct msgb *pending_msg; }; struct bsc_msc_connection *bsc_msc_create(void *ctx, struct llist_head *dest); diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h index fe8e521..7bd582c 100644 --- a/openbsc/include/openbsc/bsc_nat.h +++ b/openbsc/include/openbsc/bsc_nat.h @@ -97,6 +97,9 @@ struct bsc_connection { /* the fd we use to communicate */ struct osmo_wqueue write_queue; + /* incoming message buffer */ + struct msgb *pending_msg; + /* the BSS associated */ struct bsc_config *cfg; @@ -343,6 +346,8 @@ struct bsc_nat_ussd_con { struct bsc_nat *nat; int authorized; + struct msgb *pending_msg; + struct osmo_timer_list auth_timeout; }; diff --git a/openbsc/include/openbsc/control_cmd.h b/openbsc/include/openbsc/control_cmd.h index cd7b7b6..1217375 100644 --- a/openbsc/include/openbsc/control_cmd.h +++ b/openbsc/include/openbsc/control_cmd.h @@ -39,6 +39,9 @@ struct ctrl_connection { /* The queue for sending data back */ struct osmo_wqueue write_queue; + /* Buffer for partial input data */ + struct msgb *pending_msg; + /* Callback if the connection was closed */ void (*closed_cb)(struct ctrl_connection *conn); diff --git a/openbsc/src/libbsc/bsc_msc.c b/openbsc/src/libbsc/bsc_msc.c index 1a0f78a..a24efab 100644 --- a/openbsc/src/libbsc/bsc_msc.c +++ b/openbsc/src/libbsc/bsc_msc.c @@ -42,6 +42,13 @@ static void connection_loss(struct bsc_msc_connection *con) fd = &con->write_queue.bfd; + if (con->pending_msg) { + LOGP(DMSC, LOGL_ERROR, + "MSC(%s) dropping incomplete message.\n", con->name); + msgb_free(con->pending_msg); + con->pending_msg = NULL; + } + close(fd->fd); fd->fd = -1; fd->cb = osmo_wqueue_bfd_cb; @@ -162,6 +169,9 @@ int bsc_msc_connect(struct bsc_msc_connection *con) con->is_connected = 0; + msgb_free(con->pending_msg); + con->pending_msg = NULL; + fd = &con->write_queue.bfd; fd->fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); fd->priv_nr = 1; diff --git a/openbsc/src/libctrl/control_if.c b/openbsc/src/libctrl/control_if.c index b5db31d..1bacd31 100644 --- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -270,6 +270,7 @@ static void control_close_conn(struct ctrl_connection *ccon) llist_del(&ccon->list_entry); if (ccon->closed_cb) ccon->closed_cb(ccon); + msgb_free(ccon->pending_msg); talloc_free(ccon); } @@ -287,8 +288,10 @@ static int handle_control_read(struct osmo_fd * bfd) queue = container_of(bfd, struct osmo_wqueue, bfd); ccon = container_of(queue, struct ctrl_connection, write_queue); - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &ccon->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DCTRL, LOGL_INFO, "The control connection was closed\n"); else diff --git a/openbsc/src/osmo-bsc/osmo_bsc_msc.c b/openbsc/src/osmo-bsc/osmo_bsc_msc.c index 5517d30..3d29c60 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_msc.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_msc.c @@ -246,13 +246,15 @@ static void osmo_ext_handle(struct osmo_msc_data *msc, struct msgb *msg) static int ipaccess_a_fd_cb(struct osmo_fd *bfd) { - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; struct osmo_msc_data *data = (struct osmo_msc_data *) bfd->data; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &data->msc_con->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) { LOGP(DMSC, LOGL_ERROR, "The connection to the MSC was lost.\n"); bsc_msc_lost(data->msc_con); diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index d9fc0ca..524186a 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -796,14 +796,16 @@ static void msc_send_reset(struct bsc_msc_connection *msc_con) static int ipaccess_msc_read_cb(struct osmo_fd *bfd) { struct bsc_msc_connection *msc_con; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; int ret; msc_con = (struct bsc_msc_connection *) bfd->data; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &msc_con->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DNAT, LOGL_FATAL, "The connection the MSC(%s) was lost, exiting\n", @@ -912,6 +914,13 @@ void bsc_close_connection(struct bsc_connection *connection) osmo_wqueue_clear(&connection->write_queue); llist_del(&connection->list_entry); + if (connection->pending_msg) { + LOGP(DNAT, LOGL_ERROR, "Dropping partial message on connection %d.\n", + connection->cfg->nr); + msgb_free(connection->pending_msg); + connection->pending_msg = NULL; + } + talloc_free(connection); } @@ -1206,13 +1215,15 @@ exit3: static int ipaccess_bsc_read_cb(struct osmo_fd *bfd) { struct bsc_connection *bsc = bfd->data; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; struct ipaccess_head_ext *hh_ext; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &bsc->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DNAT, LOGL_ERROR, "The connection to the BSC Nr: %d was lost. Cleaning it\n", diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index 8da8181..5f073bf 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -66,6 +66,8 @@ static void bsc_nat_ussd_destroy(struct bsc_nat_ussd_con *con) osmo_fd_unregister(&con->queue.bfd); osmo_timer_del(&con->auth_timeout); osmo_wqueue_clear(&con->queue); + + msgb_free(con->pending_msg); talloc_free(con); } @@ -117,12 +119,14 @@ static int forward_sccp(struct bsc_nat *nat, struct msgb *msg) static int ussd_read_cb(struct osmo_fd *bfd) { struct bsc_nat_ussd_con *conn = bfd->data; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &conn->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; LOGP(DNAT, LOGL_ERROR, "USSD Connection was lost.\n"); bsc_nat_ussd_destroy(conn); return -1; -- 1.7.9.5
The old ipa_msg_recv() implementation didn't support partial receive, so IPA connections got disconnected when this happened. This patch adds the handling of the temporary message buffers and uses ipa_msg_recv_buffered(). It has been successfully tested by jerlbeck with osmo-nitb and osmo-bsc. Ticket: OW#768 Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/bsc_msc.h | 2 ++ openbsc/include/openbsc/bsc_nat.h | 5 +++++ openbsc/include/openbsc/control_cmd.h | 3 +++ openbsc/src/libbsc/bsc_msc.c | 10 ++++++++++ openbsc/src/libctrl/control_if.c | 5 ++++- openbsc/src/osmo-bsc/osmo_bsc_msc.c | 6 ++++-- openbsc/src/osmo-bsc_nat/bsc_nat.c | 19 +++++++++++++++---- openbsc/src/osmo-bsc_nat/bsc_ussd.c | 8 ++++++-- 8 files changed, 49 insertions(+), 9 deletions(-) diff --git a/openbsc/include/openbsc/bsc_msc.h b/openbsc/include/openbsc/bsc_msc.h index 647f47e..0adbd26 100644 --- a/openbsc/include/openbsc/bsc_msc.h +++ b/openbsc/include/openbsc/bsc_msc.h @@ -48,6 +48,8 @@ struct bsc_msc_connection { void (*connected) (struct bsc_msc_connection *); struct osmo_timer_list reconnect_timer; struct osmo_timer_list timeout_timer; + + struct msgb *pending_msg; }; struct bsc_msc_connection *bsc_msc_create(void *ctx, struct llist_head *dest); diff --git a/openbsc/include/openbsc/bsc_nat.h b/openbsc/include/openbsc/bsc_nat.h index fe8e521..7bd582c 100644 --- a/openbsc/include/openbsc/bsc_nat.h +++ b/openbsc/include/openbsc/bsc_nat.h @@ -97,6 +97,9 @@ struct bsc_connection { /* the fd we use to communicate */ struct osmo_wqueue write_queue; + /* incoming message buffer */ + struct msgb *pending_msg; + /* the BSS associated */ struct bsc_config *cfg; @@ -343,6 +346,8 @@ struct bsc_nat_ussd_con { struct bsc_nat *nat; int authorized; + struct msgb *pending_msg; + struct osmo_timer_list auth_timeout; }; diff --git a/openbsc/include/openbsc/control_cmd.h b/openbsc/include/openbsc/control_cmd.h index 725dce0..8aede15 100644 --- a/openbsc/include/openbsc/control_cmd.h +++ b/openbsc/include/openbsc/control_cmd.h @@ -39,6 +39,9 @@ struct ctrl_connection { /* The queue for sending data back */ struct osmo_wqueue write_queue; + /* Buffer for partial input data */ + struct msgb *pending_msg; + /* Callback if the connection was closed */ void (*closed_cb)(struct ctrl_connection *conn); diff --git a/openbsc/src/libbsc/bsc_msc.c b/openbsc/src/libbsc/bsc_msc.c index 1a0f78a..a24efab 100644 --- a/openbsc/src/libbsc/bsc_msc.c +++ b/openbsc/src/libbsc/bsc_msc.c @@ -42,6 +42,13 @@ static void connection_loss(struct bsc_msc_connection *con) fd = &con->write_queue.bfd; + if (con->pending_msg) { + LOGP(DMSC, LOGL_ERROR, + "MSC(%s) dropping incomplete message.\n", con->name); + msgb_free(con->pending_msg); + con->pending_msg = NULL; + } + close(fd->fd); fd->fd = -1; fd->cb = osmo_wqueue_bfd_cb; @@ -162,6 +169,9 @@ int bsc_msc_connect(struct bsc_msc_connection *con) con->is_connected = 0; + msgb_free(con->pending_msg); + con->pending_msg = NULL; + fd = &con->write_queue.bfd; fd->fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); fd->priv_nr = 1; diff --git a/openbsc/src/libctrl/control_if.c b/openbsc/src/libctrl/control_if.c index 2727d0d..ca59d8c 100644 --- a/openbsc/src/libctrl/control_if.c +++ b/openbsc/src/libctrl/control_if.c @@ -123,6 +123,7 @@ static void control_close_conn(struct ctrl_connection *ccon) llist_del(&ccon->list_entry); if (ccon->closed_cb) ccon->closed_cb(ccon); + msgb_free(ccon->pending_msg); talloc_free(ccon); } @@ -140,8 +141,10 @@ static int handle_control_read(struct osmo_fd * bfd) queue = container_of(bfd, struct osmo_wqueue, bfd); ccon = container_of(queue, struct ctrl_connection, write_queue); - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &ccon->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DCTRL, LOGL_INFO, "The control connection was closed\n"); else diff --git a/openbsc/src/osmo-bsc/osmo_bsc_msc.c b/openbsc/src/osmo-bsc/osmo_bsc_msc.c index 6033985..04e9cf3 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_msc.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_msc.c @@ -247,13 +247,15 @@ static void osmo_ext_handle(struct osmo_msc_data *msc, struct msgb *msg) static int ipaccess_a_fd_cb(struct osmo_fd *bfd) { - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; struct osmo_msc_data *data = (struct osmo_msc_data *) bfd->data; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &data->msc_con->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) { LOGP(DMSC, LOGL_ERROR, "The connection to the MSC was lost.\n"); bsc_msc_lost(data->msc_con); diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c index d9fc0ca..524186a 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_nat.c +++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c @@ -796,14 +796,16 @@ static void msc_send_reset(struct bsc_msc_connection *msc_con) static int ipaccess_msc_read_cb(struct osmo_fd *bfd) { struct bsc_msc_connection *msc_con; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; int ret; msc_con = (struct bsc_msc_connection *) bfd->data; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &msc_con->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DNAT, LOGL_FATAL, "The connection the MSC(%s) was lost, exiting\n", @@ -912,6 +914,13 @@ void bsc_close_connection(struct bsc_connection *connection) osmo_wqueue_clear(&connection->write_queue); llist_del(&connection->list_entry); + if (connection->pending_msg) { + LOGP(DNAT, LOGL_ERROR, "Dropping partial message on connection %d.\n", + connection->cfg->nr); + msgb_free(connection->pending_msg); + connection->pending_msg = NULL; + } + talloc_free(connection); } @@ -1206,13 +1215,15 @@ exit3: static int ipaccess_bsc_read_cb(struct osmo_fd *bfd) { struct bsc_connection *bsc = bfd->data; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; struct ipaccess_head_ext *hh_ext; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &bsc->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; if (ret == 0) LOGP(DNAT, LOGL_ERROR, "The connection to the BSC Nr: %d was lost. Cleaning it\n", diff --git a/openbsc/src/osmo-bsc_nat/bsc_ussd.c b/openbsc/src/osmo-bsc_nat/bsc_ussd.c index 8da8181..5f073bf 100644 --- a/openbsc/src/osmo-bsc_nat/bsc_ussd.c +++ b/openbsc/src/osmo-bsc_nat/bsc_ussd.c @@ -66,6 +66,8 @@ static void bsc_nat_ussd_destroy(struct bsc_nat_ussd_con *con) osmo_fd_unregister(&con->queue.bfd); osmo_timer_del(&con->auth_timeout); osmo_wqueue_clear(&con->queue); + + msgb_free(con->pending_msg); talloc_free(con); } @@ -117,12 +119,14 @@ static int forward_sccp(struct bsc_nat *nat, struct msgb *msg) static int ussd_read_cb(struct osmo_fd *bfd) { struct bsc_nat_ussd_con *conn = bfd->data; - struct msgb *msg; + struct msgb *msg = NULL; struct ipaccess_head *hh; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &conn->pending_msg); if (ret <= 0) { + if (ret == -EAGAIN) + return 0; LOGP(DNAT, LOGL_ERROR, "USSD Connection was lost.\n"); bsc_nat_ussd_destroy(conn); return -1; -- 1.7.9.5
On Mon, Mar 31, 2014 at 01:42:11PM +0200, Jacob Erlbeck wrote: Dear Jacob, I think there is a typo in the subject. Can you confirm?
It has been successfully tested by jerlbeck with osmo-nitb and osmo-bsc.
My usual question is: What has been tested? You ran the two processes and did LUs and placed calls? I think it is good to capture this information in the commit message. The patch appears fine! Great to finally have these changes. holger
On Thu, Mar 20, 2014 at 07:14:34PM +0100, Jacob Erlbeck wrote:
In addition iin case of errors the return value is now always -EXXX
Just one 'i', right?
@@ -109,6 +109,8 @@ struct e1inp_ts { struct osmo_fd fd; } rs232; } driver; + + struct msgb *pending_msg; }; @@ -27,6 +27,7 @@ struct ipa_server_conn { int (*closed_cb)(struct ipa_server_conn *peer); int (*cb)(struct ipa_server_conn *peer, struct msgb *msg); void *data; + struct msgb *pending_msg; }; @@ -53,6 +54,7 @@ struct ipa_client_conn { int (*read_cb)(struct ipa_client_conn *link, struct msgb *msg); int (*write_cb)(struct ipa_client_conn *link); void *data; + struct msgb *pending_msg; };
I think you miss a TODO-RELEASE entry?
@@ -415,13 +420,15 @@ static int handle_ts1_read(struct osmo_fd *bfd) + if (ret == -EAGAIN) + return;
The compiler warns about this return without a value. What is the correct response? The rest is looking fine.
Currently ipa_msg_recv() fails, when messages are received partially. This patch provides a new function ipa_msg_recv_buffered() that uses an additional ** to a message buffer to store partial data. When this happens, -EAGAIN is returned. If NULL is used, the function behaves similar to ipa_msg_recv() and fails on partial read. In addition in case of errors the return value is now always -EXXX and the contents of errno is undefined. Note that this feature needs support by the calling code insofar that *tmp_msg must be set to NULL initially and it must be freed and set to NULL manually when the socket is closed. Note also that ipa_msg_recv() is then a wrapper around ipa_msg_recv_buffered() which mimics the old error behaviour by setting errno explicitely to -rc and returning -1 when an error has happened. Ticket: OW#728 Sponsored-by: On-Waves ehf --- TODO-RELEASE | 1 + include/osmocom/abis/e1_input.h | 2 + include/osmocom/abis/ipa.h | 3 + src/input/ipa.c | 151 ++++++++++++++++++++++++++++++--------- src/input/ipaccess.c | 13 +++- tests/ipa_recv/ipa_recv_test.c | 55 +++++++++----- tests/ipa_recv/ipa_recv_test.ok | 27 ++++++- 7 files changed, 198 insertions(+), 54 deletions(-) diff --git a/TODO-RELEASE b/TODO-RELEASE index 43b1e8e..71931db 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -1 +1,2 @@ #library what description / commit summary line +libosmoabis abi-change ipa: Change ipa_msg_recv() to support partial receive diff --git a/include/osmocom/abis/e1_input.h b/include/osmocom/abis/e1_input.h index 9b77893..cf8677b 100644 --- a/include/osmocom/abis/e1_input.h +++ b/include/osmocom/abis/e1_input.h @@ -109,6 +109,8 @@ struct e1inp_ts { struct osmo_fd fd; } rs232; } driver; + + struct msgb *pending_msg; }; struct gsm_e1_subslot { diff --git a/include/osmocom/abis/ipa.h b/include/osmocom/abis/ipa.h index d577d74..982b694 100644 --- a/include/osmocom/abis/ipa.h +++ b/include/osmocom/abis/ipa.h @@ -27,6 +27,7 @@ struct ipa_server_conn { int (*closed_cb)(struct ipa_server_conn *peer); int (*cb)(struct ipa_server_conn *peer, struct msgb *msg); void *data; + struct msgb *pending_msg; }; struct ipa_server_conn *ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, int (*cb)(struct ipa_server_conn *peer, struct msgb *msg), int (*closed_cb)(struct ipa_server_conn *peer), void *data); @@ -53,6 +54,7 @@ struct ipa_client_conn { int (*read_cb)(struct ipa_client_conn *link, struct msgb *msg); int (*write_cb)(struct ipa_client_conn *link); void *data; + struct msgb *pending_msg; }; struct ipa_client_conn *ipa_client_conn_create(void *ctx, struct e1inp_ts *ts, int priv_nr, const char *addr, uint16_t port, void (*updown)(struct ipa_client_conn *link, int), int (*read_cb)(struct ipa_client_conn *link, struct msgb *msgb), int (*write_cb)(struct ipa_client_conn *link), void *data); @@ -64,6 +66,7 @@ void ipa_client_conn_close(struct ipa_client_conn *link); void ipa_client_conn_send(struct ipa_client_conn *link, struct msgb *msg); int ipa_msg_recv(int fd, struct msgb **rmsg); +int ipa_msg_recv_buffered(int fd, struct msgb **rmsg, struct msgb **tmp_msg); int ipaccess_rcvmsg_base(struct msgb *msg, struct osmo_fd *bfd); diff --git a/src/input/ipa.c b/src/input/ipa.c index b5abd36..71e1227 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -49,50 +49,130 @@ void ipa_msg_push_header(struct msgb *msg, uint8_t proto) int ipa_msg_recv(int fd, struct msgb **rmsg) { - struct msgb *msg; + int rc = ipa_msg_recv_buffered(fd, rmsg, NULL); + if (rc < 0) { + errno = -rc; + rc = -1; + } + return rc; +} + +int ipa_msg_recv_buffered(int fd, struct msgb **rmsg, struct msgb **tmp_msg) +{ + struct msgb *msg = tmp_msg ? *tmp_msg : NULL; struct ipaccess_head *hh; int len, ret; + int needed; - msg = ipa_msg_alloc(0); if (msg == NULL) - return -ENOMEM; + msg = ipa_msg_alloc(0); - /* first read our 3-byte header */ - hh = (struct ipaccess_head *) msg->data; - ret = recv(fd, msg->data, sizeof(*hh), 0); - if (ret <= 0) { - msgb_free(msg); - return ret; - } else if (ret != sizeof(*hh)) { - LOGP(DLINP, LOGL_ERROR, "too small message received\n"); - msgb_free(msg); - return -EIO; + if (msg == NULL) { + ret = -ENOMEM; + goto discard_msg; } - msgb_put(msg, ret); + + if (msg->l2h == NULL) { + /* first read our 3-byte header */ + needed = sizeof(*hh) - msg->len; + ret = recv(fd, msg->tail, needed, 0); + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + if (msg->len == 0) { + ret = -EAGAIN; + goto discard_msg; + } + + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message header (%d/%d)\n", + msg->len, sizeof(*hh)); + if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } + + msg->l2h = msg->tail; + } + + hh = (struct ipaccess_head *) msg->data; /* then read the length as specified in header */ - msg->l2h = msg->data + sizeof(*hh); len = ntohs(hh->len); if (len < 0 || IPA_ALLOC_SIZE < len + sizeof(*hh)) { LOGP(DLINP, LOGL_ERROR, "bad message length of %d bytes, " - "received %d bytes\n", len, ret); - msgb_free(msg); - return -EIO; + "received %d bytes\n", len, msg->len); + ret = -EIO; + goto discard_msg; } - ret = recv(fd, msg->l2h, len, 0); - if (ret <= 0) { - msgb_free(msg); - return ret; - } else if (ret < len) { - LOGP(DLINP, LOGL_ERROR, "truncated message received\n"); - msgb_free(msg); - return -EIO; + needed = len - msgb_l2len(msg); + + if (needed > 0) { + ret = recv(fd, msg->tail, needed, 0); + + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message L2 data (%d/%d)\n", + msgb_l2len(msg), len); + if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } } - msgb_put(msg, ret); + + ret = msgb_l2len(msg); + + if (ret == 0) { + LOGP(DLINP, LOGL_INFO, + "Discarding IPA message without payload\n"); + ret = -EAGAIN; + goto discard_msg; + } + + if (tmp_msg) + *tmp_msg = NULL; *rmsg = msg; return ret; + +discard_msg: + if (tmp_msg) + *tmp_msg = NULL; + msgb_free(msg); + return ret; } void ipa_client_conn_close(struct ipa_client_conn *link) @@ -103,6 +183,8 @@ void ipa_client_conn_close(struct ipa_client_conn *link) close(link->ofd->fd); link->ofd->fd = -1; } + msgb_free(link->pending_msg); + link->pending_msg = NULL; } static void ipa_client_read(struct ipa_client_conn *link) @@ -113,11 +195,12 @@ static void ipa_client_read(struct ipa_client_conn *link) LOGP(DLINP, LOGL_DEBUG, "message received\n"); - ret = ipa_msg_recv(ofd->fd, &msg); + ret = ipa_msg_recv_buffered(ofd->fd, &msg, &link->pending_msg); if (ret < 0) { - if (errno == EPIPE || errno == ECONNRESET) { + if (ret == -EAGAIN) + return; + if (ret == -EPIPE || ret == -ECONNRESET) LOGP(DLINP, LOGL_ERROR, "lost connection with server\n"); - } ipa_client_conn_close(link); if (link->updown_cb) link->updown_cb(link, 0); @@ -382,11 +465,12 @@ static void ipa_server_conn_read(struct ipa_server_conn *conn) LOGP(DLINP, LOGL_DEBUG, "message received\n"); - ret = ipa_msg_recv(ofd->fd, &msg); + ret = ipa_msg_recv_buffered(ofd->fd, &msg, &conn->pending_msg); if (ret < 0) { - if (errno == EPIPE || errno == ECONNRESET) { + if (ret == -EAGAIN) + return; + if (ret == -EPIPE || ret == -ECONNRESET) LOGP(DLINP, LOGL_ERROR, "lost connection with server\n"); - } ipa_server_conn_destroy(conn); return; } else if (ret == 0) { @@ -471,6 +555,7 @@ ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, void ipa_server_conn_destroy(struct ipa_server_conn *conn) { close(conn->ofd.fd); + msgb_free(conn->pending_msg); osmo_fd_unregister(&conn->ofd); if (conn->closed_cb) conn->closed_cb(conn); diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 225d70c..7ac5ad1 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -258,6 +258,8 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, static int ipaccess_drop(struct osmo_fd *bfd, struct e1inp_line *line) { int ret = 1; + unsigned int ts_nr = bfd->priv_nr; + struct e1inp_ts *e1i_ts = &line->ts[ts_nr-1]; /* Error case: we did not see any ID_RESP yet for this socket. */ if (bfd->fd != -1) { @@ -269,6 +271,9 @@ static int ipaccess_drop(struct osmo_fd *bfd, struct e1inp_line *line) ret = -ENOENT; } + msgb_free(e1i_ts->pending_msg); + e1i_ts->pending_msg = NULL; + /* e1inp_sign_link_destroy releases the socket descriptors for us. */ line->ops->sign_link_down(line); @@ -415,13 +420,15 @@ static int handle_ts1_read(struct osmo_fd *bfd) struct e1inp_ts *e1i_ts = &line->ts[ts_nr-1]; struct e1inp_sign_link *link; struct ipaccess_head *hh; - struct msgb *msg; + struct msgb *msg = NULL; int ret; - ret = ipa_msg_recv(bfd->fd, &msg); + ret = ipa_msg_recv_buffered(bfd->fd, &msg, &e1i_ts->pending_msg); if (ret < 0) { + if (ret == -EAGAIN) + return 0; LOGP(DLINP, LOGL_NOTICE, "Sign link problems, " - "closing socket. Reason: %s\n", strerror(errno)); + "closing socket. Reason: %s\n", strerror(-ret)); goto err; } else if (ret == 0) { LOGP(DLINP, LOGL_NOTICE, "Sign link vanished, dead socket\n"); diff --git a/tests/ipa_recv/ipa_recv_test.c b/tests/ipa_recv/ipa_recv_test.c index 7b26259..8cdc7e2 100644 --- a/tests/ipa_recv/ipa_recv_test.c +++ b/tests/ipa_recv/ipa_recv_test.c @@ -86,7 +86,7 @@ static void append_ipa_message(struct msgb *msg, int proto, const char *text) strcpy((char *)l2, text); } -static int receive_messages(int fd) +static int receive_messages(int fd, struct msgb **pending_msg) { struct msgb *msg; char dummy; @@ -97,13 +97,22 @@ static int receive_messages(int fd) break; } msg = NULL; - rc = ipa_msg_recv(fd, &msg); - if (rc == -1) - rc = -errno; + rc = ipa_msg_recv_buffered(fd, &msg, pending_msg); + fprintf(stderr, - "ipa_msg_recv: %d, msg %s NULL\n", - rc, msg ? "!=" : "=="); - if (rc == -EAGAIN) + "ipa_msg_recv_buffered: %d, msg %s NULL, " + "pending_msg %s NULL\n", + rc, msg ? "!=" : "==", + !pending_msg ? "??" : *pending_msg ? "!=" : "=="); + if (pending_msg && !!msg == !!*pending_msg) + printf( "got msg %s NULL, pending_msg %s NULL, " + "returned: %s\n", + msg ? "!=" : "==", + *pending_msg ? "!=" : "==", + rc == 0 ? "EOF" : + rc > 0 ? "OK" : + strerror(-rc)); + else if (!pending_msg && rc == -EAGAIN) printf( "got msg %s NULL, " "returned: %s\n", msg ? "!=" : "==", @@ -115,7 +124,8 @@ static int receive_messages(int fd) if (rc == -EAGAIN) break; if (rc < 0) { - printf("ipa_msg_recv failed with: %s\n", strerror(-rc)); + printf("ipa_msg_recv_buffered failed with: %s\n", + strerror(-rc)); return rc; } printf("got IPA message, size=%d, proto=%d, text=\"%s\"\n", @@ -142,13 +152,15 @@ static int slurp_data(int fd) { return count; }; -static void test_complete_recv(void) +static void test_complete_recv(int do_not_assemble) { int sv[2]; struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + struct msgb *pending_msg = NULL; int rc, i; - printf("Testing IPA recv with complete messages.\n"); + printf("Testing IPA recv with complete messages%s.\n", + do_not_assemble ? "" : " with assembling enabled"); if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) err(1, "socketpair"); @@ -166,7 +178,11 @@ static void test_complete_recv(void) } for (i=0; i < ARRAY_SIZE(ipa_test_messages); i++) { - rc = receive_messages(sv[0]); + rc = receive_messages(sv[0], + do_not_assemble ? NULL : &pending_msg); + if (pending_msg) + printf("Unexpected partial message: size=%d\n", + pending_msg->len); if (rc == 0) break; @@ -181,16 +197,19 @@ static void test_complete_recv(void) close(sv[0]); msgb_free(msg_out); + msgb_free(pending_msg); } -static void test_partial_recv(void) +static void test_partial_recv(int do_not_assemble) { int sv[2]; struct msgb *msg_out = msgb_alloc(4096, "msg_out"); + struct msgb *pending_msg = NULL; int rc, i; - printf("Testing IPA recv with partitioned messages.\n"); + printf("Testing IPA recv with partitioned messages%s.\n", + do_not_assemble ? "" : " with assembling enabled"); if (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) == -1) err(1, "socketpair"); @@ -211,7 +230,8 @@ static void test_partial_recv(void) if (msg_out->len == 0) shutdown(sv[1], SHUT_WR); - rc = receive_messages(sv[0]); + rc = receive_messages(sv[0], + do_not_assemble ? NULL : &pending_msg); if (rc == 0) break; @@ -226,6 +246,7 @@ static void test_partial_recv(void) close(sv[0]); msgb_free(msg_out); + msgb_free(pending_msg); } static struct log_info info = {}; @@ -239,8 +260,10 @@ int main(int argc, char **argv) printf("Testing the IPA layer.\n"); /* run the tests */ - test_complete_recv(); - test_partial_recv(); + test_complete_recv(1); + test_partial_recv(1); + test_complete_recv(0); + test_partial_recv(0); printf("No crashes.\n"); return 0; diff --git a/tests/ipa_recv/ipa_recv_test.ok b/tests/ipa_recv/ipa_recv_test.ok index 4144d47..bdbfb7d 100644 --- a/tests/ipa_recv/ipa_recv_test.ok +++ b/tests/ipa_recv/ipa_recv_test.ok @@ -5,8 +5,31 @@ got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMN got IPA message, size=16, proto=200, text="Hello again IPA" got IPA message, size=1, proto=200, text="" got IPA message, size=14, proto=200, text="Next is empty" -done: unread 14, unsent 0 +got msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 Testing IPA recv with partitioned messages. -ipa_msg_recv failed with: Input/output error +ipa_msg_recv_buffered failed with: Input/output error done: unread 0, unsent 154 +Testing IPA recv with complete messages with assembling enabled. +got IPA message, size=10, proto=200, text="Hello IPA" +got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz" +got IPA message, size=16, proto=200, text="Hello again IPA" +got IPA message, size=1, proto=200, text="" +got IPA message, size=14, proto=200, text="Next is empty" +got msg == NULL, pending_msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 +Testing IPA recv with partitioned messages with assembling enabled. +got IPA message, size=10, proto=200, text="Hello IPA" +got IPA message, size=86, proto=200, text="A longer test message. ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789abcdefghijklmnopqrstuvwxyz" +got IPA message, size=16, proto=200, text="Hello again IPA" +got IPA message, size=1, proto=200, text="" +got IPA message, size=14, proto=200, text="Next is empty" +got msg == NULL, pending_msg == NULL, returned: Resource temporarily unavailable +got IPA message, size=4, proto=200, text="Bye" +got IPA message, size=4, proto=200, text="Bye" +done: unread 0, unsent 0 No crashes. -- 1.7.9.5
On Mon, Mar 31, 2014 at 10:53:32AM +0200, Jacob Erlbeck wrote: Dear Jacob,
+ /* first read our 3-byte header */ + needed = sizeof(*hh) - msg->len; + ret = recv(fd, msg->tail, needed, 0); + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + if (msg->len == 0) { + ret = -EAGAIN; + goto discard_msg; + } + + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message header (%d/%d)\n", + msg->len, sizeof(*hh));
^ tab vs. spaces?
+ if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } + + msg->l2h = msg->tail; + }
+ if (needed > 0) { + ret = recv(fd, msg->tail, needed, 0); + + if (ret == 0) + goto discard_msg; + + if (ret < 0) { + if (errno == EAGAIN || errno == EINTR) + ret = 0; + else { + ret = -errno; + goto discard_msg; + } + } + + msgb_put(msg, ret); + + if (ret < needed) { + LOGP(DLINP, LOGL_INFO, + "Received part of IPA message L2 data (%d/%d)\n", + msgb_l2len(msg), len); + if (!tmp_msg) { + ret = -EIO; + goto discard_msg; + } + *tmp_msg = msg; + return -EAGAIN; + } }
Do you think readability would be improved if these two paths could be united? I will push and make releases today for this new feature.
participants (2)
-
Holger Hans Peter Freyther -
Jacob Erlbeck