laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36062?usp=email ) Change subject: osmo_io: Change struct osmo_io_ops to contain struct, not union ...................................................................... osmo_io: Change struct osmo_io_ops to contain struct, not union As we introduce more modes, and each mode aliases call-back function pointers to those of another mode, we have more and more error cases where we (for exampele) access read_cb, but in reality the user has populated recvfrom_cb. Let's use a struct, meaning that call-backs of one mode no longer alias to the same memory locations of call-backs fro another mode. This allows us to properly check if the user actually provided the right callbacks for the given mode of the iofd. This breaks ABI, but luckily not API. So a simple recompile of higher-layer library + application code will work. Change-Id: I9d302df8d00369e7b30437a52deb205f75882be3 --- M TODO-RELEASE M include/osmocom/core/osmo_io.h M src/core/osmo_io.c M src/core/osmo_io_poll.c 4 files changed, 96 insertions(+), 36 deletions(-) Approvals: laforge: Looks good to me, approved Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve diff --git a/TODO-RELEASE b/TODO-RELEASE index a8e812b..3e75184 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -12,6 +12,8 @@ core ADD osmo_sock_sctp_get_peer_addr_info() core ADD gsmtap_inst_fd2() core, DEPRECATE gsmtap_inst_fd() core behavior change osmo_tdef_fsm_inst_state_chg(): allow millisecond precision +core ABI change osmo_io_ops now contains a struct of structs, not union of structs +core ABI change osmo_iofd_set_ioops() now returns a value (error code) isdn ABI change add states and flags for external T200 handling isdn ADD initial implementation of the V.110 Terminal Adapter gsm ABI change add T200 timer states to lapdm_datalink diff --git a/include/osmocom/core/osmo_io.h b/include/osmocom/core/osmo_io.h index 46e1a53..3c704be 100644 --- a/include/osmocom/core/osmo_io.h +++ b/include/osmocom/core/osmo_io.h @@ -35,37 +35,35 @@ { return get_value_string(osmo_io_backend_names, val); } struct osmo_io_ops { - union { - /* mode OSMO_IO_FD_MODE_READ_WRITE: */ - struct { - /*! call-back function when something was read from fd */ - void (*read_cb)(struct osmo_io_fd *iofd, int res, struct msgb *msg); - /*! call-back function when write has completed on fd */ - void (*write_cb)(struct osmo_io_fd *iofd, int res, - struct msgb *msg); - /*! call-back function to segment the data at message boundaries. - * Needs to return the size of the next message. If it returns - * -EAGAIN or a value larger than msgb_length() (message is incomplete) - * osmo_io will wait for more data to be read. Other negative values - * cause the msg to be discarded. - * If a full message was received (segmentation_cb() returns a value <= msgb_length()) - * the msgb will be trimmed to size by osmo_io and forwarded to the read call-back. Any - * parsing done to the msgb by segmentation_cb() will be preserved for the read_cb() - * (e.g. setting lxh or msgb->cb). */ - int (*segmentation_cb)(struct msgb *msg); - }; + /* mode OSMO_IO_FD_MODE_READ_WRITE: */ + struct { + /*! call-back function when something was read from fd */ + void (*read_cb)(struct osmo_io_fd *iofd, int res, struct msgb *msg); + /*! call-back function when write has completed on fd */ + void (*write_cb)(struct osmo_io_fd *iofd, int res, + struct msgb *msg); + /*! call-back function to segment the data at message boundaries. + * Needs to return the size of the next message. If it returns + * -EAGAIN or a value larger than msgb_length() (message is incomplete) + * osmo_io will wait for more data to be read. Other negative values + * cause the msg to be discarded. + * If a full message was received (segmentation_cb() returns a value <= msgb_length()) + * the msgb will be trimmed to size by osmo_io and forwarded to the read call-back. Any + * parsing done to the msgb by segmentation_cb() will be preserved for the read_cb() + * (e.g. setting lxh or msgb->cb). */ + int (*segmentation_cb)(struct msgb *msg); + }; - /* mode OSMO_IO_FD_MODE_RECVFROM_SENDTO: */ - struct { - /*! call-back function emulating recvfrom */ - void (*recvfrom_cb)(struct osmo_io_fd *iofd, int res, - struct msgb *msg, - const struct osmo_sockaddr *saddr); - /*! call-back function emulating sendto */ - void (*sendto_cb)(struct osmo_io_fd *iofd, int res, - struct msgb *msg, - const struct osmo_sockaddr *daddr); - }; + /* mode OSMO_IO_FD_MODE_RECVFROM_SENDTO: */ + struct { + /*! call-back function emulating recvfrom */ + void (*recvfrom_cb)(struct osmo_io_fd *iofd, int res, + struct msgb *msg, + const struct osmo_sockaddr *saddr); + /*! call-back function emulating sendto */ + void (*sendto_cb)(struct osmo_io_fd *iofd, int res, + struct msgb *msg, + const struct osmo_sockaddr *daddr); }; }; @@ -98,4 +96,4 @@ const char *osmo_iofd_get_name(const struct osmo_io_fd *iofd); void osmo_iofd_set_name(struct osmo_io_fd *iofd, const char *name); -void osmo_iofd_set_ioops(struct osmo_io_fd *iofd, const struct osmo_io_ops *ioops); +int osmo_iofd_set_ioops(struct osmo_io_fd *iofd, const struct osmo_io_ops *ioops); diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index e059f87..9de9e2e 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -306,6 +306,8 @@ int res; struct msgb *pending = NULL; + OSMO_ASSERT(iofd->mode == OSMO_IO_FD_MODE_READ_WRITE); + if (rc <= 0) { iofd->io_ops.read_cb(iofd, rc, msg); return; @@ -473,6 +475,24 @@ return 0; } +static int check_mode_callback_compat(enum osmo_io_fd_mode mode, const struct osmo_io_ops *ops) +{ + switch (mode) { + case OSMO_IO_FD_MODE_READ_WRITE: + if (ops->recvfrom_cb || ops->sendto_cb) + return false; + break; + case OSMO_IO_FD_MODE_RECVFROM_SENDTO: + if (ops->read_cb || ops->write_cb) + return false; + break; + default: + break; + } + + return true; +} + /*! Allocate and setup a new iofd. * \param[in] ctx the parent context from which to allocate * \param[in] fd the underlying system file descriptor @@ -496,6 +516,9 @@ return NULL; } + if (!check_mode_callback_compat(mode, ioops)) + return NULL; + iofd = talloc_zero(ctx, struct osmo_io_fd); if (!iofd) return NULL; @@ -543,8 +566,10 @@ return rc; IOFD_FLAG_UNSET(iofd, IOFD_FLAG_CLOSED); - if (iofd->io_ops.read_cb) + if ((iofd->mode == OSMO_IO_FD_MODE_READ_WRITE && iofd->io_ops.read_cb) || + (iofd->mode == OSMO_IO_FD_MODE_RECVFROM_SENDTO && iofd->io_ops.recvfrom_cb)) { osmo_iofd_ops.read_enable(iofd); + } if (iofd->tx_queue.current_length > 0) osmo_iofd_ops.write_enable(iofd); @@ -722,8 +747,11 @@ /*! Set the osmo_io_ops for an iofd. * \param[in] iofd Target iofd file descriptor * \param[in] ioops osmo_io_ops structure to be set */ -void osmo_iofd_set_ioops(struct osmo_io_fd *iofd, const struct osmo_io_ops *ioops) +int osmo_iofd_set_ioops(struct osmo_io_fd *iofd, const struct osmo_io_ops *ioops) { + if (!check_mode_callback_compat(iofd->mode, ioops)) + return -EINVAL; + iofd->io_ops = *ioops; switch (iofd->mode) { @@ -743,6 +771,8 @@ default: OSMO_ASSERT(0); } + + return 0; } /*! Notify the user if/when the socket is connected. diff --git a/src/core/osmo_io_poll.c b/src/core/osmo_io_poll.c index 5000dca..d7f80c0 100644 --- a/src/core/osmo_io_poll.c +++ b/src/core/osmo_io_poll.c @@ -81,10 +81,18 @@ rc = sendmsg(ofd->fd, &msghdr->hdr, msghdr->flags); iofd_handle_send_completion(iofd, rc, msghdr); } else { - if (iofd->mode == OSMO_IO_FD_MODE_READ_WRITE) - /* Socket is writable, but we have no data to send. A non-blocking/async - connect() is signalled this way. */ + /* Socket is writable, but we have no data to send. A non-blocking/async + connect() is signalled this way. */ + switch (iofd->mode) { + case OSMO_IO_FD_MODE_READ_WRITE: iofd->io_ops.write_cb(iofd, 0, NULL); + break; + case OSMO_IO_FD_MODE_RECVFROM_SENDTO: + iofd->io_ops.sendto_cb(iofd, 0, NULL, NULL); + break; + default: + break; + } if (osmo_iofd_txqueue_len(iofd) == 0) iofd_poll_ops.write_disable(iofd); } -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36062?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I9d302df8d00369e7b30437a52deb205f75882be3 Gerrit-Change-Number: 36062 Gerrit-PatchSet: 2 Gerrit-Owner: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: jolly &lt;andreas(a)eversberg.eu&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged