Hello,
attached are two patches I came across while looking at the pablo/ctrl-updates branch in OpenBSC.
The first one is just a bugfix for a resource leak and the second one adds a possibility to register a callback that will notify you if the connection is lost. This is needed for the control interface as we'll need to clean up some ctrlif related things if a connection is lost.
Be aware that I haven't tested this with the ctrl-updates branch since this doesn't compile yet. I did test that master still compiles and osmo-bsc/nat don't segfault on start (though openbsc doesn't seem to use ipa_server_conn at the moment). The e1inp_ipa_{bsc,bts}_test worked as well.
Regards,
Daniel Willmann (2): ipa: Fix resource leak if we encounter an error in ipa_server_conn_read ipa: Add a callback to detect if the server_conn was closed
include/osmocom/abis/ipa.h | 3 ++- src/input/ipa.c | 6 +++++- src/ipa_proxy.c | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-)
In case of a connection reset or protocol error we should destroy the connection as well. --- src/input/ipa.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/input/ipa.c b/src/input/ipa.c index 2441de4..c881b22 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -390,6 +390,7 @@ static void ipa_server_conn_read(struct ipa_server_conn *conn) if (errno == EPIPE || errno == ECONNRESET) { LOGP(DLINP, LOGL_ERROR, "lost connection with server\n"); } + ipa_server_conn_destroy(conn); return; } else if (ret == 0) { LOGP(DLINP, LOGL_ERROR, "connection closed with server\n");
--- include/osmocom/abis/ipa.h | 3 ++- src/input/ipa.c | 5 ++++- src/ipa_proxy.c | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/osmocom/abis/ipa.h b/include/osmocom/abis/ipa.h index 43422dc..397cf9c 100644 --- a/include/osmocom/abis/ipa.h +++ b/include/osmocom/abis/ipa.h @@ -24,11 +24,12 @@ struct ipa_server_conn { struct ipa_server_link *server; struct osmo_fd ofd; struct llist_head tx_queue; + int (*closed_cb)(struct ipa_server_conn *peer); int (*cb)(struct ipa_server_conn *peer, struct msgb *msg); void *data; };
-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), void *data); +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); void ipa_server_conn_destroy(struct ipa_server_conn *peer);
void ipa_server_conn_send(struct ipa_server_conn *peer, struct msgb *msg); diff --git a/src/input/ipa.c b/src/input/ipa.c index c881b22..a887959 100644 --- a/src/input/ipa.c +++ b/src/input/ipa.c @@ -443,7 +443,7 @@ static int ipa_server_conn_cb(struct osmo_fd *ofd, unsigned int what) struct ipa_server_conn * ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, int (*cb)(struct ipa_server_conn *conn, struct msgb *msg), - void *data) + int (*closed_cb)(struct ipa_server_conn *conn), void *data) { struct ipa_server_conn *conn;
@@ -459,6 +459,7 @@ ipa_server_conn_create(void *ctx, struct ipa_server_link *link, int fd, conn->ofd.cb = ipa_server_conn_cb; conn->ofd.when = BSC_FD_READ; conn->cb = cb; + conn->closed_cb = closed_cb; conn->data = data; INIT_LLIST_HEAD(&conn->tx_queue);
@@ -474,6 +475,8 @@ void ipa_server_conn_destroy(struct ipa_server_conn *conn) { close(conn->ofd.fd); osmo_fd_unregister(&conn->ofd); + if (conn->closed_cb) + conn->closed_cb(conn); talloc_free(conn); }
diff --git a/src/ipa_proxy.c b/src/ipa_proxy.c index f4e1df8..9816d02 100644 --- a/src/ipa_proxy.c +++ b/src/ipa_proxy.c @@ -176,7 +176,7 @@ ipa_sock_src_accept_cb(struct ipa_server_link *link, int fd) conn->route = route;
conn->src = ipa_server_conn_create(tall_ipa_proxy_ctx, link, fd, - ipa_sock_src_cb, conn); + ipa_sock_src_cb, NULL, conn); if (conn->src == NULL) { LOGP(DLINP, LOGL_ERROR, "could not create server peer: %s\n", strerror(errno));
On Thu, Sep 15, 2011 at 12:56:56PM +0200, Daniel Willmann wrote:
Hello,
attached are two patches I came across while looking at the pablo/ctrl-updates branch in OpenBSC.
The first one is just a bugfix for a resource leak and the second one adds a possibility to register a callback that will notify you if the connection is lost. This is needed for the control interface as we'll need to clean up some ctrlif related things if a connection is lost.
Thanks for the patches, nice catch the leak fix.
I'm fine with the API change to add the close callback, I think we're still in time to make this changes without much pain.
@Harald: Could you apply these patches? I can do it myself if you want. Let me know.
On 09/16/2011 01:12 AM, Pablo Neira Ayuso wrote:
On Thu, Sep 15, 2011 at 12:56:56PM +0200, Daniel Willmann wrote:
Thanks for the patches, nice catch the leak fix.
This fix reminded me that we should have some way to test for 'leaks', semi automatically. What I have done so far with On-Waves when deploying/branching was to have a setup with two phones (Motorola A1200, Nokia ...), some crazy chat script to call ATD, AT, send SMS from the Motorola phone, have another script that 'scripts' telnet to either drop OML/RSL and on top of that have another script that is using tcpkill, sending SIGKILL2 to drop the MSC connection...
Leak checking was done by using the talloc reporting (it also shows that not every context hangs of the right roots), using top -p `pidof ...`. It would be nice if we could automate this and extend it to the various external interfaces (e.g. CTRL).
cheers
On Fri, Sep 16, 2011 at 01:20:56AM +0200, Holger Hans Peter Freyther wrote:
On 09/16/2011 01:12 AM, Pablo Neira Ayuso wrote:
On Thu, Sep 15, 2011 at 12:56:56PM +0200, Daniel Willmann wrote:
Thanks for the patches, nice catch the leak fix.
This fix reminded me that we should have some way to test for 'leaks', semi automatically. What I have done so far with On-Waves when deploying/branching was to have a setup with two phones (Motorola A1200, Nokia ...), some crazy chat script to call ATD, AT, send SMS from the Motorola phone, have another script that 'scripts' telnet to either drop OML/RSL and on top of that have another script that is using tcpkill, sending SIGKILL2 to drop the MSC connection...
Leak checking was done by using the talloc reporting (it also shows that not every context hangs of the right roots), using top -p `pidof ...`. It would be nice if we could automate this and extend it to the various external interfaces (e.g. CTRL).
I see, automating leak checking seems hard to achieve. In other tools that I have, I added some command to display something similar to the talloc reporting so I could use it to check if my program was leaking at some pool (and if so, reviewing the code that allocates memory in that pool).
Probably we can add that command to display the talloc report to the generic VTY interface in libosmocore.
Hi Pablo and Daniel,
On Fri, Sep 16, 2011 at 01:12:18AM +0200, Pablo Neira Ayuso wrote:
I'm fine with the API change to add the close callback, I think we're still in time to make this changes without much pain.
I'm afraid there will be one more round of libosmo-abis related changes in not too distant future, adding (and hiding) support for a new signalling protocol that != ip.access.
@Harald: Could you apply these patches? I can do it myself if you want. Let me know.
I'm fine with the patches, feel free to apply them.
On Fri, Sep 16, 2011 at 07:58:38AM +0100, Harald Welte wrote:
On Fri, Sep 16, 2011 at 01:12:18AM +0200, Pablo Neira Ayuso wrote:
I'm fine with the API change to add the close callback, I think we're still in time to make this changes without much pain.
I'm afraid there will be one more round of libosmo-abis related changes in not too distant future, adding (and hiding) support for a new signalling protocol that != ip.access.
Ok, let's see.
@Harald: Could you apply these patches? I can do it myself if you want. Let me know.
I'm fine with the patches, feel free to apply them.
I have applied both patches to libosmo-abis. Thanks!
P.S: I still have to take your patches for my openBSC branch, I expect to do it by tomorrow.
-
Daniel Willmann -
Harald Welte -
Holger Hans Peter Freyther -
Pablo Neira Ayuso