From: Pablo Neira Ayuso pablo@netfilter.org
Hi,
The following patchset contain updates for libosmo-abis:
* fix for leak of e1inp_line reported by Holger. * more robust handling of error codes in several parts of the ipaccess driver. * minor cleanups for LAPD and dahdi, fix some compilations warnings. * generic LAPD PCAP interface to create PCAP files containing the LAPD frames, so far it was only possible to store the OML/RSL traces, without the LAPD frames. * fix crash of IPA BTS example test
If you're OK with those, let me know. I can push them myself if you want or you can manually apply them.
Pablo Neira Ayuso (11): ipa: fix missing set of write_cb for IPA client connection ipaccess: fix leak of e1inp_line tests: e1inp_ipa_bsc_test: fix crash ipaccess: more robust error handle in receival path ipaccess: more robust error handling in the accept patch ipaccess: more robust error handling in the send path tests: e1inp_ipa_bts_test: fix compilation warnings lapd: use C99 structure initialization for profile templates input: dahdi: use logging facilities instead of fprintf and stderr input: dahdi: replace exit by return input: add generic PCAP interface for LAPD
include/Makefile.am | 2 +- include/osmocom/abis/lapd.h | 1 + include/osmocom/abis/lapd_pcap.h | 11 +++ src/Makefile.am | 1 + src/input/dahdi.c | 54 ++++++++----- src/input/ipa.c | 13 +++- src/input/ipaccess.c | 107 ++++++++++++++++++++----- src/input/lapd.c | 66 +++++++++------- src/input/lapd_pcap.c | 160 ++++++++++++++++++++++++++++++++++++++ tests/e1inp_ipa_bsc_test.c | 8 +- tests/e1inp_ipa_bts_test.c | 7 +- 11 files changed, 353 insertions(+), 77 deletions(-) create mode 100644 include/osmocom/abis/lapd_pcap.h create mode 100644 src/input/lapd_pcap.c
From: Pablo Neira Ayuso pablo@gnumonks.org
Now the BTS and BSC tests work again. --- src/input/ipa.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/input/ipa.c b/src/input/ipa.c index a887959..3c6a507 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -249,6 +249,9 @@ ipa_client_conn_create(void *ctx, struct e1inp_ts *ts, /* default to generic write callback if not set. */ if (write_cb == NULL) ipa_link->write_cb = ipa_client_write_default_cb; + else + ipa_link->write_cb = write_cb; + if (ts) ipa_link->line = ts->line; ipa_link->data = data;
On Wed, Aug 22, 2012 at 05:04:43PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
Now the BTS and BSC tests work again.
could you look into integrating your test with the GNU autotest testsuite we use in libosmocore/libosmo-sccp/openbsc? This way we would catch this issue at build time.
if (write_cb == NULL) ipa_link->write_cb = ipa_client_write_default_cb;
- else
ipa_link->write_cb = write_cb;
this means that only the tests (and no real application) set a custom write_cb? is there another way to test?
On Thu, Aug 23, 2012 at 09:03:21AM +0200, Holger Hans Peter Freyther wrote:
On Wed, Aug 22, 2012 at 05:04:43PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Hi,
Now the BTS and BSC tests work again.
could you look into integrating your test with the GNU autotest testsuite we use in libosmocore/libosmo-sccp/openbsc? This way we would catch this issue at build time.
I will. Would you be OK if I do that in some follow-up patchset?
if (write_cb == NULL) ipa_link->write_cb = ipa_client_write_default_cb;
- else
ipa_link->write_cb = write_cb;this means that only the tests (and no real application) set a custom write_cb? is there another way to test?
It's ipaccess driver which is setting its own write_cb to integrate itself with the e1inp queue of messages that you get via e1inp_tx_ts.
On Thu, Aug 23, 2012 at 07:17:32PM +0200, Pablo Neira Ayuso wrote:
I will. Would you be OK if I do that in some follow-up patchset?
Yes, no rush.
It's ipaccess driver which is setting its own write_cb to integrate itself with the e1inp queue of messages that you get via e1inp_tx_ts.
The following is not urgent, so feel free to use the patch as is. With every test one attempts to simulate reality. In the above case the reality was okay, the simulation of it was broken. The question (and not for now) is if the test can be modified to be more close to reality. Or at least remove the write_cb parameter and set it from outside.
holger
From: Pablo Neira Ayuso pablo@gnumonks.org
Holger reported a leak in the ipaccess_drop path. It seems I forgot to call e1inp_line_put twice, one for the OML link and one for the RSL link.
Note that, for some fully established A-bis IPA link, the refcnt of the e1inp_line becomes two. --- src/input/ipaccess.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 76d1994..44a5a59 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -250,7 +250,10 @@ static int ipaccess_drop(struct osmo_fd *bfd) ret = -ENOENT; } /* put the virtual E1 line that we cloned for this socket, if - * it becomes unused, it gets released. */ + * it becomes unused, it gets released. We have to make it + * twice: once for the OML link and once for the RSL link + */ + e1inp_line_put(line); e1inp_line_put(line); return ret; }
On Wed, Aug 22, 2012 at 05:04:44PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Holger reported a leak in the ipaccess_drop path.
New version of this patch, this time based on Holger's plus handling the case in which no full OML / RSL has been established.
On Wed, Aug 22, 2012 at 07:44:19PM +0200, Pablo Neira Ayuso wrote:
On Wed, Aug 22, 2012 at 05:04:44PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Holger reported a leak in the ipaccess_drop path.
New version of this patch, this time based on Holger's plus handling the case in which no full OML / RSL has been established.
You obviously know this code a lot more than I do. The manual test for it is. I will test it but probably not today.
1.) Use the drop bts ... command in OpenBSC... drop the RSL link, drop the OML link.. check if there is a leak or crash. 2.) Force the bts to reconnect (either by using tcpkill or such) or by... and check if there is a leak.
From: Pablo Neira Ayuso pablo@gnumonks.org
The sequence to trigger the crash was:
1) establish a full A-bis IPA link (both OML and RSL correctly established and ID_RESP received from BTS). 2) nc 127.0.0.1 3002 # establish OML link only
I forgot to set to NULL the oml and rsl links we're using. Otherwise, the test calls e1inp_sign_link_destroy(rsl) which does not exists. --- tests/e1inp_ipa_bsc_test.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/e1inp_ipa_bsc_test.c b/tests/e1inp_ipa_bsc_test.c index 424c87b..a0e5653 100644 --- a/tests/e1inp_ipa_bsc_test.c +++ b/tests/e1inp_ipa_bsc_test.c @@ -62,10 +62,14 @@ sign_link_up(void *dev, struct e1inp_line *line, enum e1inp_sign_type type) static void sign_link_down(struct e1inp_line *line) { LOGP(DBSCTEST, LOGL_NOTICE, "signal link has been closed\n"); - if (oml_sign_link) + if (oml_sign_link) { e1inp_sign_link_destroy(oml_sign_link); - if (rsl_sign_link) + oml_sign_link = NULL; + } + if (rsl_sign_link) { e1inp_sign_link_destroy(rsl_sign_link); + rsl_sign_link = NULL; + } }
static void fill_om_hdr(struct abis_om_hdr *oh, uint8_t len)
From: Pablo Neira Ayuso pablo@gnumonks.org
If we hit any error in the receival path of the ipaccess input driver, then better spot an error containing the reason and close the socket to start over. --- src/input/ipaccess.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 44a5a59..31b8f04 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -387,9 +387,13 @@ static int handle_ts1_read(struct osmo_fd *bfd) int ret = 0, error;
error = ipa_msg_recv(bfd->fd, &msg); - if (error < 0) + if (error < 0) { + if (ipaccess_drop(bfd) >= 0) { + LOGP(DLINP, LOGL_NOTICE, "Sign link problems, " + "closing socket. Reason: %s\n", strerror(errno)); + } return error; - else if (error == 0) { + } else if (error == 0) { if (ipaccess_drop(bfd) >= 0) { LOGP(DLINP, LOGL_NOTICE, "Sign link vanished, " "dead socket\n");
From: Pablo Neira Ayuso pablo@gnumonks.org
Better close the newly established TCP connection to force a re-establishment. --- src/input/ipaccess.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 31b8f04..c69ae24 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -601,15 +601,22 @@ static int ipaccess_bsc_oml_cb(struct ipa_server_link *link, int fd) ret = osmo_fd_register(bfd); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "could not register FD\n"); - close(bfd->fd); - e1inp_line_put(line); - return ret; + goto err; }
/* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */ ret = ipaccess_send_id_req(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n", + strerror(errno)); + goto err; + }
- return ret; + return ret; +err: + close(bfd->fd); + e1inp_line_put(line); + return ret; }
static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) @@ -645,14 +652,21 @@ static int ipaccess_bsc_rsl_cb(struct ipa_server_link *link, int fd) ret = osmo_fd_register(bfd); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "could not register FD\n"); - close(bfd->fd); - e1inp_line_put(line); - return ret; + goto err; } /* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */ ret = ipaccess_send_id_req(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n", + strerror(errno)); + goto err; + }
return 0; +err: + close(bfd->fd); + e1inp_line_put(line); + return ret; }
static struct msgb *
On Wed, Aug 22, 2012 at 05:04:47PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
Better close the newly established TCP connection to force a re-establishment.
two things.
a.) I think you can do the cleanup/codesharing in more paths.
ret = osmo_fd_register(bfd); if (ret < 0) { LOGP(DLINP, LOGL_ERROR, "could not register FD\n");
close(bfd->fd);e1inp_line_put(line);return ret;
goto err;}
/* Request ID. FIXME: request LOCATION, HW/SW VErsion, Unit Name, Serno */ ret = ipaccess_send_id_req(bfd->fd);
if (ret < 0) {
LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n",strerror(errno));goto err;}
the bfd remains registered!
- if (ret < 0) {
LOGP(DLINP, LOGL_ERROR, "could not send ID REQ. Reason: %s\n",strerror(errno));goto err;- }
same bfd issue.
On Wed, Aug 22, 2012 at 05:04:47PM +0200, pablo@gnumonks.org wrote:
+err:
- close(bfd->fd);
- e1inp_line_put(line);
- return ret;
}
one more thing. The code in ipaccess_drop appears to set bfd->fd = -1. Do we need that? You certainly don't have to do this in the current patchset but it would be nice if we at least know if we should (re)initialize to -1 or not.
holger
From: Pablo Neira Ayuso pablo@gnumonks.org
In case of problems to transmit message, spot an error and close the socket to force a re-establishment.
This also modifies ipa_client_read to close the socket in case that our callback reports an error. --- src/input/ipa.c | 10 ++++++-- src/input/ipaccess.c | 66 +++++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 63 insertions(+), 13 deletions(-)
diff --git a/src/input/ipa.c b/src/input/ipa.c index 3c6a507..015d957 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -131,8 +131,14 @@ static void ipa_client_read(struct ipa_client_conn *link) ipa_client_retry(link); return; } - if (link->read_cb) - link->read_cb(link, msg); + if (link->read_cb) { + ret = link->read_cb(link, msg); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "connection closed with " + "server\n"); + ipa_client_retry(link); + } + } }
static void ipa_client_write(struct ipa_client_conn *link) diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index c69ae24..e59ab18 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -189,26 +189,33 @@ int ipaccess_send_id_req(int fd) /* base handling of the ip.access protocol */ int ipaccess_rcvmsg_base(struct msgb *msg, struct osmo_fd *bfd) { - int ipa_ccm = 0; uint8_t msg_type = *(msg->l2h); int ret = 0;
switch (msg_type) { case IPAC_MSGT_PING: - ipa_ccm = 1; ret = ipaccess_send_pong(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "cannot send PING " + "message. Reason: %s\n", strerror(errno)); + } else + ret = 1; break; case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); - ipa_ccm = 1; + ret = 1; break; case IPAC_MSGT_ID_ACK: DEBUGP(DLMI, "ID_ACK? -> ACK!\n"); - ipa_ccm = 1; ret = ipaccess_send_id_ack(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK " + "message. Reason: %s\n", strerror(errno)); + } else + ret = 1; break; } - return ipa_ccm; + return ret; }
/* base handling of the ip.access protocol */ @@ -221,6 +228,10 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, switch (msg_type) { case IPAC_MSGT_PING: ret = ipaccess_send_pong(bfd->fd); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "cannot send PONG " + "message. Reason: %s\n", strerror(errno)); + } break; case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); @@ -229,7 +240,7 @@ int ipaccess_rcvmsg_bts_base(struct msgb *msg, DEBUGP(DLMI, "ID_ACK\n"); break; } - return 0; + return ret; }
static int ipaccess_drop(struct osmo_fd *bfd) @@ -269,8 +280,15 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, int len, ret;
/* Handle IPA PING, PONG and ID_ACK messages. */ - if (ipaccess_rcvmsg_base(msg, bfd)) + ret = ipaccess_rcvmsg_base(msg, bfd); + switch(ret) { + case -1: + /* error in IPA control message handling */ + goto err; + case 1: + /* this is an IPA control message, skip further processing */ return 0; + }
switch (msg_type) { case IPAC_MSGT_ID_RESP: @@ -517,6 +535,16 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) DEBUGP(DLMI, "TX %u: %s\n", ts_nr, osmo_hexdump(msg->l2h, msgb_l2len(msg)));
ret = send(bfd->fd, msg->data, msg->len, 0); + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "failed to send A-bis IPA signalling " + "message. Reason: %s\n", strerror(errno)); + if (ipaccess_drop(bfd) >= 0) { + LOGP(DLINP, LOGL_NOTICE, "Sign link problems, " + "closing socket. Reason: %s\n", strerror(errno)); + return ret; + } + } + msgb_free(msg);
/* set tx delay timer for next event */ @@ -759,13 +787,16 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) struct ipaccess_head *hh = (struct ipaccess_head *) msg->data; struct e1inp_ts *e1i_ts = NULL; struct e1inp_sign_link *sign_link; + int ret = 0;
/* special handling for IPA CCM. */ if (hh->proto == IPAC_PROTO_IPACCESS) { uint8_t msg_type = *(msg->l2h);
/* ping, pong and acknowledgment cases. */ - ipaccess_rcvmsg_bts_base(msg, link->ofd); + ret = ipaccess_rcvmsg_bts_base(msg, link->ofd); + if (ret < 0) + return ret;
/* this is a request for identification from the BSC. */ if (msg_type == IPAC_MSGT_ID_GET) { @@ -798,15 +829,28 @@ static int ipaccess_bts_cb(struct ipa_client_conn *link, struct msgb *msg) } rmsg = ipa_bts_id_resp(link->line->ops->cfg.ipa.dev, data + 1, len - 1); - ipaccess_send(link->ofd->fd, rmsg->data, rmsg->len); + ret = ipaccess_send(link->ofd->fd, rmsg->data, + rmsg->len); msgb_free(rmsg);
+ if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "cannot send ID_RESP " + "message. Reason: %s\n", strerror(errno)); + return ret; + } + /* send ID_ACK. */ rmsg = ipa_bts_id_ack(); - ipaccess_send(link->ofd->fd, rmsg->data, rmsg->len); + ret = ipaccess_send(link->ofd->fd, rmsg->data, + rmsg->len); msgb_free(rmsg); + + if (ret < 0) { + LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK " + "message. Reason: %s\n", strerror(errno)); + } } - return 0; + return ret; } else if (link->port == IPA_TCP_PORT_OML) e1i_ts = &link->line->ts[0]; else if (link->port == IPA_TCP_PORT_RSL)
On Wed, Aug 22, 2012 at 05:04:48PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
In case of problems to transmit message, spot an error and close the socket to force a re-establishment.
This also modifies ipa_client_read to close the socket in case that our callback reports an error.
this somehow doesn't read right. Sorry to be picky here. :}
LOGP(DLINP, LOGL_ERROR, "connection closed with ""server\n");
I try to have full sentences in the log message. This means it starts with a capital letter and ends with a full stop.
int ret = 0;
int ret = 1 ;
switch (msg_type) { case IPAC_MSGT_PING:
ipa_ccm = 1;
if (ret < 0) {LOGP(DLINP, LOGL_ERROR, "cannot send PING ""message. Reason: %s\n", strerror(errno));
}
break;case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); break; case IPAC_MSGT_ID_ACK: DEBUGP(DLMI, "ID_ACK? -> ACK!\n");
ipa_ccm = 1;
if (ret < 0) {LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK ""message. Reason: %s\n", strerror(errno));
}
break;
default: ret = 0; break;
}
- return ipa_ccm;
- return ret;
}
does this read any better?
static int ipaccess_drop(struct osmo_fd *bfd) @@ -269,8 +280,15 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, int len, ret;
/* Handle IPA PING, PONG and ID_ACK messages. */
- if (ipaccess_rcvmsg_base(msg, bfd))
- ret = ipaccess_rcvmsg_base(msg, bfd);
- switch(ret) {
- case -1:
/* error in IPA control message handling */goto err;- case 1:
/* this is an IPA control message, skip further processing */
default: WARN_ON(ret); ?
}
switch (msg_type) { case IPAC_MSGT_ID_RESP:
@@ -517,6 +535,16 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) DEBUGP(DLMI, "TX %u: %s\n", ts_nr, osmo_hexdump(msg->l2h, msgb_l2len(msg)));
ret = send(bfd->fd, msg->data, msg->len, 0);
- if (ret < 0) {
if (ret != msg->len)
On Thu, Aug 23, 2012 at 09:19:17AM +0200, Holger Hans Peter Freyther wrote:
On Wed, Aug 22, 2012 at 05:04:48PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
In case of problems to transmit message, spot an error and close the socket to force a re-establishment.
This also modifies ipa_client_read to close the socket in case that our callback reports an error.
this somehow doesn't read right. Sorry to be picky here. :}
No problem, I'll fix the description.
LOGP(DLINP, LOGL_ERROR, "connection closed with ""server\n");I try to have full sentences in the log message. This means it starts with a capital letter and ends with a full stop.
int ret = 0;
int ret = 1 ;
This will not help, ret is overwritten by ipaccess_send_*, I need to reload it anyway for the PING and ID_ACK cases after the send().
I have changed this to:
int ret;
And below, inlined the changes I've done for the new patch version:
switch (msg_type) { case IPAC_MSGT_PING:
ipa_ccm = 1;
if (ret < 0) {LOGP(DLINP, LOGL_ERROR, "cannot send PING ""message. Reason: %s\n", strerror(errno));
break;
}
ret = 1;
break;case IPAC_MSGT_PONG: DEBUGP(DLMI, "PONG!\n"); break; case IPAC_MSGT_ID_ACK: DEBUGP(DLMI, "ID_ACK? -> ACK!\n");
ipa_ccm = 1;
if (ret < 0) {LOGP(DLINP, LOGL_ERROR, "cannot send ID_ACK ""message. Reason: %s\n", strerror(errno));
break;
}
ret = 1;
break;default: ret = 0; break;
}
- return ipa_ccm;
- return ret;
}
does this read any better?
If you refer to remove ipa_ccm and to use ret instead, well, it's just that I have merged ret and ipa_ccm, and ipa_ccm = send(...) seems ugly to me.
If you refer to the entire function, yes I think it reads better now.
static int ipaccess_drop(struct osmo_fd *bfd) @@ -269,8 +280,15 @@ static int ipaccess_rcvmsg(struct e1inp_line *line, struct msgb *msg, int len, ret;
/* Handle IPA PING, PONG and ID_ACK messages. */
- if (ipaccess_rcvmsg_base(msg, bfd))
- ret = ipaccess_rcvmsg_base(msg, bfd);
- switch(ret) {
- case -1:
/* error in IPA control message handling */goto err;- case 1:
/* this is an IPA control message, skip further processing */default: WARN_ON(ret); ?
Added case 0 (this is not an IPA control message) and default case using LOGP to report an error.
}
switch (msg_type) { case IPAC_MSGT_ID_RESP:
@@ -517,6 +535,16 @@ static int __handle_ts1_write(struct osmo_fd *bfd, struct e1inp_line *line) DEBUGP(DLMI, "TX %u: %s\n", ts_nr, osmo_hexdump(msg->l2h, msgb_l2len(msg)));
ret = send(bfd->fd, msg->data, msg->len, 0);
- if (ret < 0) {
if (ret != msg->len)
I've changed all send callers to check for ret != msg->len. Note that if we ever support TCP segmentation (TCP_NODELAY unset), then we'll have to relax that checking (although I don't think we'll ever do, unless for the ipaccess BTS).
Thanks for you review Holger.
On Thu, Aug 23, 2012 at 09:19:17AM +0200, Holger Hans Peter Freyther wrote:
On Wed, Aug 22, 2012 at 05:04:48PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
In case of problems to transmit message, spot an error and close the socket to force a re-establishment.
This also modifies ipa_client_read to close the socket in case that our callback reports an error.
this somehow doesn't read right. Sorry to be picky here. :}
New version of this patch attached.
From: Pablo Neira Ayuso pablo@gnumonks.org
CC e1inp_ipa_bts_test.o e1inp_ipa_bts_test.c: In function ‘sign_link_up’: e1inp_ipa_bts_test.c:47:8: warning: variable ‘dst’ set but not used [-Wunused-but-set-variable] e1inp_ipa_bts_test.c: In function ‘test_bts_gsm_12_21_cb’: e1inp_ipa_bts_test.c:211:6: warning: variable ‘ret’ set but not used [-Wunused-but-set-variable] CCLD e1inp_ipa_bts_test --- tests/e1inp_ipa_bts_test.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/e1inp_ipa_bts_test.c b/tests/e1inp_ipa_bts_test.c index 3549661..bf46e9e 100644 --- a/tests/e1inp_ipa_bts_test.c +++ b/tests/e1inp_ipa_bts_test.c @@ -44,7 +44,6 @@ static struct e1inp_sign_link * sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) { struct e1inp_sign_link *sign_link = NULL; - void *dst = NULL;
switch(type) { case E1INP_SIGN_OML: @@ -58,7 +57,6 @@ sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) LOGP(DBTSTEST, LOGL_ERROR, "cannot create OML sign link\n"); } - dst = oml_sign_link; if (oml_sign_link) { unsigned int event_type = 0;
@@ -84,7 +82,6 @@ sign_link_up(void *unit, struct e1inp_line *line, enum e1inp_sign_type type) LOGP(DBTSTEST, LOGL_ERROR, "cannot create RSL sign link\n"); } - dst = rsl_sign_link; break; default: return NULL; @@ -221,6 +218,10 @@ static int test_bts_gsm_12_21_cb(struct osmo_fd *ofd, unsigned int what) unit->bts_id, unit->trx_id, 0, NULL, 0); + if (ret < 0) { + LOGP(DBTSTEST, LOGL_ERROR, "cannot send SW ACT REQ\n"); + break; + } bts_state = BTS_TEST_OML_WAIT_SW_ACT_ACK; break; case BTS_TEST_OML_WAIT_SW_ACT_ACK:
From: Pablo Neira Ayuso pablo@gnumonks.org
--- src/input/lapd.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/src/input/lapd.c b/src/input/lapd.c index a7a86ce..5a475c4 100644 --- a/src/input/lapd.c +++ b/src/input/lapd.c @@ -71,39 +71,39 @@ #define LAPD_SET_K(n, o) {n,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}
const struct lapd_profile lapd_profile_isdn = { - LAPD_SET_K(7,7), - 3, - 260, - 3, - 1,0, - 1,0, - 2,0, - 10,0, - 0 + .k = LAPD_SET_K(7,7), + .n200 = 3, + .n201 = 260, + .n202 = 3, + .t200_sec = 1, .t200_usec = 0, + .t201_sec = 1, .t201_usec = 0, + .t202_sec = 2, .t202_usec = 0, + .t203_sec = 10, .t203_usec = 0, + .short_address = 0 };
const struct lapd_profile lapd_profile_abis = { - LAPD_SET_K(2,1), - 3, - 260, - 0, /* infinite */ - 0,240000, - 1,0, - 2,0, - 10,0, - 0 + .k = LAPD_SET_K(2,1), + .n200 = 3, + .n201 = 260, + .n202 = 0, /* infinite */ + .t200_sec = 0, .t200_usec = 240000, + .t201_sec = 1, .t201_usec = 0, + .t202_sec = 2, .t202_usec = 0, + .t203_sec = 10, .t203_usec = 0, + .short_address = 0 };
const struct lapd_profile lapd_profile_sat = { - LAPD_SET_K(15,15), - 5, - 260, - 5, - 2,400000, - 2,400000, - 2,400000, - 20,0, - 1 + .k = LAPD_SET_K(15,15), + .n200 = 5, + .n201 = 260, + .n202 = 5, + .t200_sec = 2, .t200_usec = 400000, + .t201_sec = 2, .t201_usec = 400000, + .t202_sec = 2, .t202_usec = 400000, + .t203_sec = 20, .t203_sec = 0, + .short_address = 1 };
typedef enum {
From: Pablo Neira Ayuso pablo@gnumonks.org
--- src/input/dahdi.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/src/input/dahdi.c b/src/input/dahdi.c index 0cefa6c..ff5cb80 100644 --- a/src/input/dahdi.c +++ b/src/input/dahdi.c @@ -196,7 +196,7 @@ static int ts_want_write(struct e1inp_ts *e1i_ts) * writeset, since it doesn't support poll() based * write flow control */ if (e1i_ts->type == E1INP_TS_TYPE_TRAU) { - fprintf(stderr, "Trying to write TRAU ts\n"); + LOGP(DLINP, LOGL_DEBUG, "Trying to write TRAU ts\n"); return 0; }
@@ -302,7 +302,7 @@ static int handle_tsX_write(struct osmo_fd *bfd) ret = subchan_mux_out(mx, tx_buf, D_BCHAN_TX_GRAN);
if (ret != D_BCHAN_TX_GRAN) { - fprintf(stderr, "Huh, got ret of %d\n", ret); + LOGP(DLINP, LOGL_DEBUG, "Huh, got ret of %d\n", ret); if (ret < 0) return ret; } @@ -316,8 +316,8 @@ static int handle_tsX_write(struct osmo_fd *bfd)
ret = write(bfd->fd, tx_buf, ret); if (ret < D_BCHAN_TX_GRAN) - fprintf(stderr, "send returns %d instead of %d\n", ret, - D_BCHAN_TX_GRAN); + LOGP(DLINP, LOGL_DEBUG, "send returns %d instead of %d\n", + ret, D_BCHAN_TX_GRAN);
return ret; } @@ -337,7 +337,8 @@ static int handle_tsX_read(struct osmo_fd *bfd)
ret = read(bfd->fd, msg->data, D_TSX_ALLOC_SIZE); if (ret < 0 || ret != D_TSX_ALLOC_SIZE) { - fprintf(stderr, "read error %d %s\n", ret, strerror(errno)); + LOGP(DLINP, LOGL_DEBUG, "read error %d %s\n", + ret, strerror(errno)); return ret; }
@@ -388,7 +389,8 @@ static int dahdi_fd_cb(struct osmo_fd *bfd, unsigned int what) * write flow control */ break; default: - fprintf(stderr, "unknown E1 TS type %u\n", e1i_ts->type); + LOGP(DLINP, LOGL_NOTICE, + "unknown E1 TS type %u\n", e1i_ts->type); break; }
@@ -429,7 +431,7 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) int x = 0;
if (ioctl(fd, DAHDI_GET_BUFINFO, &bi)) { - fprintf(stderr, "Error getting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error getting bufinfo\n"); exit(-1); }
@@ -443,13 +445,13 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) }
if (ioctl(fd, DAHDI_SET_BUFINFO, &bi)) { - fprintf(stderr, "Error setting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); exit(-1); }
if (!as_sigchan) { if (ioctl(fd, DAHDI_AUDIOMODE, &x)) { - fprintf(stderr, "Error setting bufinfo\n"); + LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); exit(-1); } } else { @@ -517,7 +519,8 @@ static int dahdi_e1_setup(struct e1inp_line *line) if (!bfd->fd) bfd->fd = open(openstr, O_RDWR | O_NONBLOCK); if (bfd->fd == -1) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); exit(-1); } @@ -537,7 +540,8 @@ static int dahdi_e1_setup(struct e1inp_line *line) if (!bfd->fd) bfd->fd = open(openstr, O_RDWR | O_NONBLOCK); if (bfd->fd == -1) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); exit(-1); } @@ -550,14 +554,16 @@ static int dahdi_e1_setup(struct e1inp_line *line) }
if (bfd->fd < 0) { - fprintf(stderr, "%s could not open %s %s\n", + LOGP(DLINP, LOGL_ERROR, + "%s could not open %s %s\n", __func__, openstr, strerror(errno)); return bfd->fd; }
ret = osmo_fd_register(bfd); if (ret < 0) { - fprintf(stderr, "could not register FD: %s\n", + LOGP(DLINP, LOGL_ERROR, + "could not register FD: %s\n", strerror(ret)); return ret; }
From: Pablo Neira Ayuso pablo@gnumonks.org
This is a library, we leave up to the client code to decide when to finish the code execution. --- src/input/dahdi.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/input/dahdi.c b/src/input/dahdi.c index ff5cb80..8d3e060 100644 --- a/src/input/dahdi.c +++ b/src/input/dahdi.c @@ -425,14 +425,14 @@ struct e1inp_driver dahdi_driver = { .vty_show = &dahdi_vty_show, };
-void dahdi_set_bufinfo(int fd, int as_sigchan) +int dahdi_set_bufinfo(int fd, int as_sigchan) { struct dahdi_bufferinfo bi; int x = 0;
if (ioctl(fd, DAHDI_GET_BUFINFO, &bi)) { LOGP(DLINP, LOGL_ERROR, "Error getting bufinfo\n"); - exit(-1); + return -EIO; }
if (as_sigchan) { @@ -446,13 +446,13 @@ void dahdi_set_bufinfo(int fd, int as_sigchan)
if (ioctl(fd, DAHDI_SET_BUFINFO, &bi)) { LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); - exit(-1); + return -EIO; }
if (!as_sigchan) { if (ioctl(fd, DAHDI_AUDIOMODE, &x)) { LOGP(DLINP, LOGL_ERROR, "Error setting bufinfo\n"); - exit(-1); + return -EIO; } } else { int one = 1; @@ -461,6 +461,7 @@ void dahdi_set_bufinfo(int fd, int as_sigchan) * as this command will fail if the slot _already_ was a * signalling slot before :( */ } + return 0; }
static int dahdi_e1_setup(struct e1inp_line *line) @@ -522,10 +523,13 @@ static int dahdi_e1_setup(struct e1inp_line *line) LOGP(DLINP, LOGL_ERROR, "%s could not open %s %s\n", __func__, openstr, strerror(errno)); - exit(-1); + return -EIO; } bfd->when = BSC_FD_READ | BSC_FD_EXCEPT; - dahdi_set_bufinfo(bfd->fd, 1); + ret = dahdi_set_bufinfo(bfd->fd, 1); + if (ret < 0) + return ret; + if (!e1i_ts->lapd) e1i_ts->lapd = lapd_instance_alloc(1, dahdi_write_msg, bfd, e1inp_dlsap_up, @@ -543,9 +547,11 @@ static int dahdi_e1_setup(struct e1inp_line *line) LOGP(DLINP, LOGL_ERROR, "%s could not open %s %s\n", __func__, openstr, strerror(errno)); - exit(-1); + return -EIO; } - dahdi_set_bufinfo(bfd->fd, 0); + ret = dahdi_set_bufinfo(bfd->fd, 0); + if (ret < 0) + return -EIO; /* We never include the DAHDI B-Channel FD into the * writeset, since it doesn't support poll() based * write flow control */
On Wed, Aug 22, 2012 at 05:04:52PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
This is a library, we leave up to the client code to decide when to finish the code execution.
from a non-native speaker... "we leave it up"..
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch allows you to create PCAP traces between the BSC and BTS including the LAPD frames. Useful for debugging communication issues.
So far, it was only possible to create layer 3 traces containing OML/RSL. LAPD traces can be also interesting to debug communication issues between the BSC and the BTS.
To enable PCAP of LAPD, you only have to invoke:
li->pcap_fd = osmo_pcap_lapd_open("/tmp/file.pcap", 0600);
By default, li->pcap_fd is set to -1 which means disabled.
openBSC needs a patch to replace all usage of e1_set_pcap_fd by osmo_pcap_lapd_open. --- include/Makefile.am | 2 +- include/osmocom/abis/lapd.h | 1 + include/osmocom/abis/lapd_pcap.h | 11 +++ src/Makefile.am | 1 + src/input/lapd.c | 12 +++ src/input/lapd_pcap.c | 160 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 186 insertions(+), 1 deletion(-) create mode 100644 include/osmocom/abis/lapd_pcap.h create mode 100644 src/input/lapd_pcap.c
diff --git a/include/Makefile.am b/include/Makefile.am index 61e6cf5..16fa506 100644 --- a/include/Makefile.am +++ b/include/Makefile.am @@ -3,4 +3,4 @@ noinst_HEADERS=mISDNif.h internal.h nobase_include_HEADERS = osmocom/abis/ipa.h osmocom/abis/trau_frame.h \ osmocom/abis/ipa_proxy.h osmocom/abis/ipaccess.h osmocom/abis/abis.h \ osmocom/abis/subchan_demux.h osmocom/abis/e1_input.h \ - osmocom/abis/lapd.h osmocom/trau/osmo_ortp.h + osmocom/abis/lapd.h osmocom/abis/lapd_pcap.h osmocom/trau/osmo_ortp.h diff --git a/include/osmocom/abis/lapd.h b/include/osmocom/abis/lapd.h index 847a597..2457cde 100644 --- a/include/osmocom/abis/lapd.h +++ b/include/osmocom/abis/lapd.h @@ -35,6 +35,7 @@ struct lapd_instance { struct lapd_profile profile; /* must be a copy */
struct llist_head tei_list; /* list of TEI in this LAPD instance */ + int pcap_fd; /* PCAP file descriptor */ };
enum lapd_recv_errors { diff --git a/include/osmocom/abis/lapd_pcap.h b/include/osmocom/abis/lapd_pcap.h new file mode 100644 index 0000000..1c0d555 --- /dev/null +++ b/include/osmocom/abis/lapd_pcap.h @@ -0,0 +1,11 @@ +#ifndef _LAPD_PCAP_H_ +#define _LAPD_PCAP_H_ + +#define OSMO_LAPD_PCAP_INPUT 0 +#define OSMO_LAPD_PCAP_OUTPUT 1 + +int osmo_pcap_lapd_open(char *filename, mode_t mode); +int osmo_pcap_lapd_write(int fd, int direction, struct msgb *msg); +int osmo_pcap_lapd_close(int fd); + +#endif diff --git a/src/Makefile.am b/src/Makefile.am index cf58aaa..01f0913 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -23,6 +23,7 @@ libosmoabis_la_SOURCES = init.c \ input/ipa.c \ input/ipaccess.c \ input/lapd.c \ + input/lapd_pcap.c \ input/misdn.c \ input/rs232.c
diff --git a/src/input/lapd.c b/src/input/lapd.c index 5a475c4..bced94a 100644 --- a/src/input/lapd.c +++ b/src/input/lapd.c @@ -36,6 +36,7 @@ #include <osmocom/core/msgb.h> #include <osmocom/core/timer.h> #include <osmocom/abis/lapd.h> +#include <osmocom/abis/lapd_pcap.h>
#define LAPD_ADDR2(sapi, cr) ((((sapi) & 0x3f) << 2) | (((cr) & 0x1) << 1)) #define LAPD_ADDR3(tei) ((((tei) & 0x7f) << 1) | 0x1) @@ -311,6 +312,10 @@ static int lapd_tei_receive(struct lapd_instance *li, uint8_t *data, int len) msg = msgb_alloc_headroom(56, 56, "DL EST"); msg->l2h = msgb_push(msg, 8); memcpy(msg->l2h, resp, 8); + + /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_OUTPUT, msg); + LOGP(DLLAPD, LOGL_DEBUG, "TX: %s\n", osmo_hexdump(msg->data, msg->len)); li->transmit_cb(msg, li->transmit_cbdata); @@ -336,6 +341,9 @@ int lapd_receive(struct lapd_instance *li, struct msgb *msg, int *error) struct lapd_sap *sap; struct lapd_tei *teip;
+ /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_INPUT, msg); + LOGP(DLLAPD, LOGL_DEBUG, "RX: %s\n", osmo_hexdump(msg->data, msg->len)); if (msg->len < 2) { LOGP(DLLAPD, LOGL_ERROR, "LAPD frame receive len %d < 2, " @@ -585,6 +593,9 @@ static int send_ph_data_req(struct lapd_msg_ctx *lctx, struct msgb *msg) else msg->l2h[1] = LAPD_ADDR3(lctx->tei);
+ /* write to PCAP file, if enabled. */ + osmo_pcap_lapd_write(li->pcap_fd, OSMO_LAPD_PCAP_OUTPUT, msg); + /* forward frame to L1 */ LOGP(DLLAPD, LOGL_DEBUG, "TX: %s\n", osmo_hexdump(msg->data, msg->len)); li->transmit_cb(msg, li->transmit_cbdata); @@ -645,6 +656,7 @@ struct lapd_instance *lapd_instance_alloc(int network_side, li->transmit_cbdata = tx_cbdata; li->receive_cb = rx_cb; li->receive_cbdata = rx_cbdata; + li->pcap_fd = -1; memcpy(&li->profile, profile, sizeof(li->profile));
INIT_LLIST_HEAD(&li->tei_list); diff --git a/src/input/lapd_pcap.c b/src/input/lapd_pcap.c new file mode 100644 index 0000000..4430687 --- /dev/null +++ b/src/input/lapd_pcap.c @@ -0,0 +1,160 @@ +/* (C) 2008-2012 by Harald Welte laforge@gnumonks.org + * + * All Rights Reserved + * + * Author: Harald Welte laforge@gnumonks.org + * Pablo Neira Ayuso pablo@gnumonks.org + * + * 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 <stdio.h> +#include <stdint.h> +#include <stddef.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> +#include <sys/time.h> +#include <arpa/inet.h> +#include <string.h> +#include <errno.h> + +#include <osmocom/core/msgb.h> +#include <osmocom/core/logging.h> +#include <osmocom/core/utils.h> + +#include <osmocom/abis/lapd_pcap.h> + +/* + * pcap writing of the mlapd load + * pcap format is from http://wiki.wireshark.org/Development/LibpcapFileFormat + */ +#define DLT_LINUX_LAPD 177 + +struct pcap_hdr { + uint32_t magic_number; + uint16_t version_major; + uint16_t version_minor; + int32_t thiszone; + uint32_t sigfigs; + uint32_t snaplen; + uint32_t network; +} __attribute__((packed)); + +struct pcap_rechdr { + uint32_t ts_sec; + uint32_t ts_usec; + uint32_t incl_len; + uint32_t orig_len; +} __attribute__((packed)); + +struct pcap_lapdhdr { + uint16_t pkttype; + uint16_t hatype; + uint16_t halen; + uint64_t addr; + int16_t protocol; +} __attribute__((packed)); + +osmo_static_assert(offsetof(struct pcap_lapdhdr, hatype) == 2, hatype_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, halen) == 4, halen_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, addr) == 6, addr_offset); +osmo_static_assert(offsetof(struct pcap_lapdhdr, protocol) == 14, proto_offset); +osmo_static_assert(sizeof(struct pcap_lapdhdr) == 16, lapd_header_size); + +int osmo_pcap_lapd_open(char *filename, mode_t mode) +{ + int fd; + struct pcap_hdr pcap_header = { + .magic_number = 0xa1b2c3d4, + .version_major = 2, + .version_minor = 4, + .thiszone = 0, + .sigfigs = 0, + .snaplen = 65535, + .network = DLT_LINUX_LAPD, + }; + + LOGP(DLLAPD, LOGL_NOTICE, "opening LAPD pcap file `%s'\n", filename); + + fd = open(filename, O_WRONLY|O_TRUNC|O_CREAT, mode); + if (fd < 0) { + LOGP(DLLAPD, LOGL_ERROR, "failed to open PCAP file: %s\n", + strerror(errno)); + return -1; + } + if (write(fd, &pcap_header, sizeof(pcap_header)) < 0) { + LOGP(DLLAPD, LOGL_ERROR, "cannot write PCAP header: %s\n", + strerror(errno)); + close(fd); + return -1; + } + return fd; +} + +/* This currently only works for the D-Channel */ +int osmo_pcap_lapd_write(int fd, int direction, struct msgb *msg) +{ + int ret; + int numbytes = 0; + struct timeval tv; + struct pcap_rechdr pcap_rechdr; + struct pcap_lapdhdr header; + char buf[sizeof(struct pcap_rechdr) + + sizeof(struct pcap_lapdhdr) + msg->len]; + + /* PCAP file has not been opened, skip. */ + if (fd < 0) + return 0; + + pcap_rechdr.ts_sec = 0; + pcap_rechdr.ts_usec = 0; + pcap_rechdr.incl_len = msg->len + sizeof(struct pcap_lapdhdr); + pcap_rechdr.orig_len = msg->len + sizeof(struct pcap_lapdhdr); + + header.pkttype = 4; + header.hatype = 0; + header.halen = 0; + header.addr = direction == OSMO_LAPD_PCAP_OUTPUT ? 0x0 : 0x1; + header.protocol = ntohs(48); + + gettimeofday(&tv, NULL); + pcap_rechdr.ts_sec = tv.tv_sec; + pcap_rechdr.ts_usec = tv.tv_usec; + + memcpy(buf + numbytes, &pcap_rechdr, sizeof(pcap_rechdr)); + numbytes += sizeof(pcap_rechdr); + + memcpy(buf + numbytes, &header, sizeof(header)); + numbytes += sizeof(header); + + memcpy(buf + numbytes, msg->data, msg->len); + numbytes += msg->len; + + ret = write(fd, buf, numbytes); + if (ret < 0) { + LOGP(DLLAPD, LOGL_ERROR, "cannot write packet to PCAP: %s\n", + strerror(errno)); + return -1; + } + return numbytes; +} + +int osmo_pcap_lapd_close(int fd) +{ + LOGP(DLLAPD, LOGL_NOTICE, "closing LAPD pcap file\n"); + return close(fd); +}
On Wed, Aug 22, 2012 at 05:04:53PM +0200, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@gnumonks.org
This patch allows you to create PCAP traces between the BSC and BTS including the LAPD frames. Useful for debugging communication issues.
nice!
- if (write(fd, &pcap_header, sizeof(pcap_header)) < 0) {
!= sizeof(pcap_header)?
- ret = write(fd, buf, numbytes);
- if (ret < 0) {
ret != numbytes?
Hi Pablo,
On Wed, Aug 22, 2012 at 05:04:42PM +0200, pablo@gnumonks.org wrote:
The following patchset contain updates for libosmo-abis:
Thanks a lot for your updates and fixes. I quickly looked through them and I don't have anything to add to Holgers review, except that I don't like a full stop at the end of log lines, like he does ;)
On Thu, Aug 23, 2012 at 11:08:18AM +0200, Harald Welte wrote:
Hi Pablo,
On Wed, Aug 22, 2012 at 05:04:42PM +0200, pablo@gnumonks.org wrote:
The following patchset contain updates for libosmo-abis:
Thanks a lot for your updates and fixes. I quickly looked through them and I don't have anything to add to Holgers review, except that I don't like a full stop at the end of log lines, like he does ;)
Hm I don't like it either. So this is a 2 vs. 1 game :-P.
Well since we have no consensus on the full stop, and there is many other logs that would need to be fixed to add the full stop, I decided not to add it.
Please, let me know if you find any other issue. Otherwise I'll push this after a couple of this with no comments.
Thanks to both of you!
On Thu, Aug 23, 2012 at 02:00:59PM +0200, Pablo Neira Ayuso wrote: [...]
Please, let me know if you find any other issue. Otherwise I'll push this after a couple of this with no comments.
JFYI: I'll resend a new patchset to the mailing list to address Holger's comments so you can give it another review.
-
Harald Welte -
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org