Change in osmo-cbc[master]: Refactor {cbsp,sbcap}_cbc_accept_cb

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-cbc/+/28712 ) Change subject: Refactor {cbsp,sbcap}_cbc_accept_cb ...................................................................... Refactor {cbsp,sbcap}_cbc_accept_cb Delay allocating the structures until really needed. First do checks agains the opened connection, and if an issue is detected simply close the fd. Next, fetch or allocate the related peer. Finally, create the link from the fd. This makes it easier to add early extra checks later when peers will contain info on link_mode (client|server|disabled) later on. Change-Id: Id4f83ec6b0b14e556b1caa9c80e7f68d062fec57 --- M include/osmocom/cbc/cbsp_link.h M include/osmocom/cbc/sbcap_link.h M src/cbsp_link.c M src/cbsp_link_fsm.c M src/sbcap_link.c M src/sbcap_link_fsm.c 6 files changed, 139 insertions(+), 80 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/cbc/cbsp_link.h b/include/osmocom/cbc/cbsp_link.h index 6699222..7fcacdf 100644 --- a/include/osmocom/cbc/cbsp_link.h +++ b/include/osmocom/cbc/cbsp_link.h @@ -40,6 +40,8 @@ struct cbc_peer *peer; }; +struct cbc_cbsp_link *cbc_cbsp_link_alloc(struct cbc_cbsp_mgr *cbc, struct cbc_peer *peer); +void cbc_cbsp_link_free(struct cbc_cbsp_link *link); const char *cbc_cbsp_link_name(const struct cbc_cbsp_link *link); void cbc_cbsp_link_tx(struct cbc_cbsp_link *link, struct osmo_cbsp_decoded *cbsp); void cbc_cbsp_link_close(struct cbc_cbsp_link *link); diff --git a/include/osmocom/cbc/sbcap_link.h b/include/osmocom/cbc/sbcap_link.h index a4618e1..5946544 100644 --- a/include/osmocom/cbc/sbcap_link.h +++ b/include/osmocom/cbc/sbcap_link.h @@ -38,6 +38,8 @@ struct cbc_peer *peer; }; +struct cbc_sbcap_link *cbc_sbcap_link_alloc(struct cbc_sbcap_mgr *cbc, struct cbc_peer *peer); +void cbc_sbcap_link_free(struct cbc_sbcap_link *link); const char *cbc_sbcap_link_name(const struct cbc_sbcap_link *link); void cbc_sbcap_link_tx(struct cbc_sbcap_link *link, SBcAP_SBC_AP_PDU_t *pdu); void cbc_sbcap_link_close(struct cbc_sbcap_link *link); diff --git a/src/cbsp_link.c b/src/cbsp_link.c index bc421f2..cd1c308 100644 --- a/src/cbsp_link.c +++ b/src/cbsp_link.c @@ -19,6 +19,7 @@ */ #include <errno.h> +#include <unistd.h> #include <sys/types.h> #include <sys/socket.h> @@ -37,6 +38,36 @@ #include <osmocom/cbc/cbsp_link_fsm.h> #include <osmocom/cbc/cbc_peer.h> +struct cbc_cbsp_link *cbc_cbsp_link_alloc(struct cbc_cbsp_mgr *cbc, struct cbc_peer *peer) +{ + struct cbc_cbsp_link *link; + + link = talloc_zero(cbc, struct cbc_cbsp_link); + OSMO_ASSERT(link); + + link->peer = peer; + + link->fi = osmo_fsm_inst_alloc(&cbsp_link_fsm, link, link, LOGL_DEBUG, NULL); + if (!link->fi) { + LOGPCC(link, LOGL_ERROR, "Unable to allocate FSM\n"); + talloc_free(link); + return NULL; + } + + llist_add_tail(&link->list, &cbc->links); + return link; +} + +void cbc_cbsp_link_free(struct cbc_cbsp_link *link) +{ + if (!link) + return; + llist_del(&link->list); + if (link->fi) + osmo_fsm_inst_free(link->fi); + talloc_free(link); +} + const char *cbc_cbsp_link_name(const struct cbc_cbsp_link *link) { OSMO_ASSERT(link); @@ -112,10 +143,10 @@ static int cbsp_cbc_accept_cb(struct osmo_stream_srv_link *srv_link, int fd) { struct cbc_cbsp_mgr *cbc = osmo_stream_srv_link_get_data(srv_link); - struct cbc_cbsp_link *link = talloc_zero(cbc, struct cbc_cbsp_link); + struct cbc_peer *peer; + struct cbc_cbsp_link *link; char remote_ip[INET6_ADDRSTRLEN], portbuf[6]; int remote_port; - OSMO_ASSERT(link); remote_ip[0] = '\0'; portbuf[0] = '\0'; @@ -124,45 +155,41 @@ LOGP(DCBSP, LOGL_NOTICE, "New CBSP link connection from %s:%u\n", remote_ip, remote_port); - link->conn = osmo_stream_srv_create(srv_link, srv_link, fd, cbsp_cbc_read_cb, cbsp_cbc_closed_cb, link); - if (!link->conn) { - LOGP(DCBSP, LOGL_ERROR, "Unable to create stream server for %s:%d\n", - remote_ip, remote_port); - talloc_free(link); - return -1; - } - link->fi = osmo_fsm_inst_alloc(&cbsp_link_fsm, link, link, LOGL_DEBUG, NULL); - if (!link->fi) { - LOGPCC(link, LOGL_ERROR, "Unable to allocate FSM\n"); - cbc_cbsp_link_close(link); - talloc_free(link); - return -1; - } - llist_add_tail(&link->list, &cbc->links); - /* Match link to peer */ - link->peer = cbc_peer_by_addr_proto(remote_ip, remote_port, CBC_PEER_PROTO_CBSP); - if (!link->peer) { - if (g_cbc->config.permit_unknown_peers) { - LOGPCC(link, LOGL_NOTICE, "Accepting unknown CBSP peer %s:%d\n", - remote_ip, remote_port); - link->peer = cbc_peer_create(NULL, CBC_PEER_PROTO_CBSP); - OSMO_ASSERT(link->peer); - link->peer->unknown_dynamic_peer = true; - } else { - LOGPCC(link, LOGL_NOTICE, "Rejecting unknown CBSP peer %s:%d (not permitted)\n", - remote_ip, remote_port); - cbc_cbsp_link_close(link); + peer = cbc_peer_by_addr_proto(remote_ip, remote_port, CBC_PEER_PROTO_CBSP); + if (!peer) { + if (!g_cbc->config.permit_unknown_peers) { + LOGP(DCBSP, LOGL_NOTICE, + "Rejecting unknown CBSP peer %s:%d (not permitted)\n", + remote_ip, remote_port); + close(fd); return -1; } - } else { - if (link->peer->link.cbsp) { - LOGPCC(link, LOGL_ERROR, "We already have a connection for peer %s\n", - link->peer->name); - /* FIXME */ - } - link->peer->link.cbsp = link; + LOGP(DCBSP, LOGL_NOTICE, "Accepting unknown CBSP peer %s:%d\n", + remote_ip, remote_port); + peer = cbc_peer_create(NULL, CBC_PEER_PROTO_CBSP); + OSMO_ASSERT(peer); + peer->unknown_dynamic_peer = true; } + if (peer->link.cbsp) { + LOGPCC(peer->link.cbsp, LOGL_ERROR, + "We already have a connection for peer %s, closing it\n", + peer->name); + cbc_cbsp_link_close(peer->link.cbsp); + } + link = cbc_cbsp_link_alloc(cbc, peer); + OSMO_ASSERT(link); + link->conn = osmo_stream_srv_create(srv_link, srv_link, fd, + cbsp_cbc_read_cb, cbsp_cbc_closed_cb, + link); + if (!link->conn) { + LOGPCC(link, LOGL_ERROR, + "Unable to create stream server for %s:%u\n", + remote_ip, remote_port); + cbc_cbsp_link_free(link); + return -1; + } + peer->link.cbsp = link; osmo_fsm_inst_dispatch(link->fi, CBSP_LINK_E_CMD_RESET, NULL); return 0; diff --git a/src/cbsp_link_fsm.c b/src/cbsp_link_fsm.c index d46a83e..eba21a7 100644 --- a/src/cbsp_link_fsm.c +++ b/src/cbsp_link_fsm.c @@ -197,13 +197,13 @@ struct cbc_cbsp_link *link = (struct cbc_cbsp_link *) fi->priv; cbc_cbsp_link_close(link); - llist_del(&link->list); - link->fi = NULL; /* reparent the fsm_inst to the cbc as we're about to free() it's talloc * parent 'link' */ + link->fi = NULL; talloc_steal(g_cbc, fi); - talloc_free(link); + + cbc_cbsp_link_free(link); } static const struct osmo_fsm_state cbsp_link_fsm_states[] = { diff --git a/src/sbcap_link.c b/src/sbcap_link.c index 4656f6d..9194509 100644 --- a/src/sbcap_link.c +++ b/src/sbcap_link.c @@ -19,6 +19,7 @@ */ #include <errno.h> +#include <unistd.h> #include <sys/types.h> #include <sys/socket.h> #include <netdb.h> @@ -41,6 +42,36 @@ #include <osmocom/cbc/cbc_peer.h> #include <osmocom/cbc/debug.h> +struct cbc_sbcap_link *cbc_sbcap_link_alloc(struct cbc_sbcap_mgr *cbc, struct cbc_peer *peer) +{ + struct cbc_sbcap_link *link; + + link = talloc_zero(cbc, struct cbc_sbcap_link); + OSMO_ASSERT(link); + + link->peer = peer; + + link->fi = osmo_fsm_inst_alloc(&sbcap_link_fsm, link, link, LOGL_DEBUG, NULL); + if (!link->fi) { + LOGPSBCAPC(link, LOGL_ERROR, "Unable to allocate FSM\n"); + talloc_free(link); + return NULL; + } + + llist_add_tail(&link->list, &cbc->links); + return link; +} + +void cbc_sbcap_link_free(struct cbc_sbcap_link *link) +{ + if (!link) + return; + llist_del(&link->list); + if (link->fi) + osmo_fsm_inst_free(link->fi); + talloc_free(link); +} + const char *cbc_sbcap_link_name(const struct cbc_sbcap_link *link) { struct osmo_fd *ofd; @@ -140,10 +171,10 @@ static int sbcap_cbc_accept_cb(struct osmo_stream_srv_link *srv_link, int fd) { struct cbc_sbcap_mgr *cbc = osmo_stream_srv_link_get_data(srv_link); - struct cbc_sbcap_link *link = talloc_zero(cbc, struct cbc_sbcap_link); + struct cbc_peer *peer; + struct cbc_sbcap_link *link; char remote_ip[INET6_ADDRSTRLEN], portbuf[6]; int remote_port; - OSMO_ASSERT(link); remote_ip[0] = '\0'; portbuf[0] = '\0'; @@ -152,45 +183,42 @@ LOGP(DSBcAP, LOGL_NOTICE, "New SBc-AP link connection from %s:%u\n", remote_ip, remote_port); - link->conn = osmo_stream_srv_create(srv_link, srv_link, fd, sbcap_cbc_read_cb, sbcap_cbc_closed_cb, link); - if (!link->conn) { - LOGP(DSBcAP, LOGL_ERROR, "Unable to create stream server for %s:%d\n", - remote_ip, remote_port); - talloc_free(link); - return -1; - } - link->fi = osmo_fsm_inst_alloc(&sbcap_link_fsm, link, link, LOGL_DEBUG, NULL); - if (!link->fi) { - LOGPSBCAPC(link, LOGL_ERROR, "Unable to allocate FSM\n"); - cbc_sbcap_link_close(link); - talloc_free(link); - return -1; - } - llist_add_tail(&link->list, &cbc->links); - /* Match link to peer */ - link->peer = cbc_peer_by_addr_proto(remote_ip, remote_port, CBC_PEER_PROTO_SBcAP); - if (!link->peer) { - if (g_cbc->config.permit_unknown_peers) { - LOGPSBCAPC(link, LOGL_NOTICE, "Accepting unknown SBc-AP peer %s:%d\n", - remote_ip, remote_port); - link->peer = cbc_peer_create(NULL, CBC_PEER_PROTO_SBcAP); - OSMO_ASSERT(link->peer); - link->peer->unknown_dynamic_peer = true; - } else { - LOGPSBCAPC(link, LOGL_NOTICE, "Rejecting unknown SBc-AP peer %s:%d (not permitted)\n", - remote_ip, remote_port); - cbc_sbcap_link_close(link); + peer = cbc_peer_by_addr_proto(remote_ip, remote_port, CBC_PEER_PROTO_SBcAP); + if (!peer) { + if (!g_cbc->config.permit_unknown_peers) { + LOGP(DSBcAP, LOGL_NOTICE, + "Rejecting unknown SBc-AP peer %s:%d (not permitted)\n", + remote_ip, remote_port); + close(fd); return -1; } - } else { - if (link->peer->link.sbcap) { - LOGPSBCAPC(link, LOGL_ERROR, "We already have a connection for peer %s\n", - link->peer->name); - /* FIXME */ - } - link->peer->link.sbcap = link; + LOGP(DSBcAP, LOGL_NOTICE, "Accepting unknown SBc-AP peer %s:%d\n", + remote_ip, remote_port); + peer = cbc_peer_create(NULL, CBC_PEER_PROTO_SBcAP); + OSMO_ASSERT(peer); + peer->unknown_dynamic_peer = true; } + if (peer->link.sbcap) { + LOGPSBCAPC(peer->link.sbcap, LOGL_ERROR, + "We already have a connection for peer %s, closing it\n", + peer->name); + cbc_sbcap_link_close(peer->link.sbcap); + } + link = cbc_sbcap_link_alloc(cbc, peer); + OSMO_ASSERT(link); + + link->conn = osmo_stream_srv_create(srv_link, srv_link, fd, + sbcap_cbc_read_cb, sbcap_cbc_closed_cb, + link); + if (!link->conn) { + LOGPSBCAPC(link, LOGL_ERROR, + "Unable to create stream server for %s:%u\n", + remote_ip, remote_port); + cbc_sbcap_link_free(link); + return -1; + } + peer->link.sbcap = link; osmo_fsm_inst_dispatch(link->fi, SBcAP_LINK_E_CMD_RESET, NULL); return 0; diff --git a/src/sbcap_link_fsm.c b/src/sbcap_link_fsm.c index 9447520..0c66a62 100644 --- a/src/sbcap_link_fsm.c +++ b/src/sbcap_link_fsm.c @@ -95,13 +95,13 @@ struct cbc_sbcap_link *link = (struct cbc_sbcap_link *) fi->priv; cbc_sbcap_link_close(link); - llist_del(&link->list); - link->fi = NULL; /* reparent the fsm_inst to the cbc as we're about to free() it's talloc * parent 'link' */ + link->fi = NULL; talloc_steal(g_cbc, fi); - talloc_free(link); + + cbc_sbcap_link_free(link); } static const struct osmo_fsm_state sbcap_link_fsm_states[] = { -- To view, visit https://gerrit.osmocom.org/c/osmo-cbc/+/28712 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-cbc Gerrit-Branch: master Gerrit-Change-Id: Id4f83ec6b0b14e556b1caa9c80e7f68d062fec57 Gerrit-Change-Number: 28712 Gerrit-PatchSet: 3 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged