pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/42354?usp=email )

Change subject: osmo_io: Rewrite iofd_handle_send_completion() to fix multiple issues ......................................................................

osmo_io: Rewrite iofd_handle_send_completion() to fix multiple issues

Previous implementation had multiple issues, which this patch addresses: * Didn't call user cb updating completed bytes for a given msg which had a partial write. * rc checks against "Incomplete write" were wrong, because rc was not being decremented with previous msgs idxs being processed. * Didn't take into account the write(0, fd, buf) = 0 valid user case for regular files.

The rewrite also simplifies the logic by splitting the "rc < 0" (error) paths from the "rc >= 0" (successful) paths.

Related: SYS#7842 Change-Id: Ia016e4df7be5e534a8212f7271caff9779e08eb1 --- M src/core/osmo_io.c 1 file changed, 56 insertions(+), 32 deletions(-)

Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve jolly: Looks good to me, but someone else must approve pespin: Looks good to me, approved

diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index 7c59a61..2a20405 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -509,6 +509,26 @@ } }

+static void iofd_send_completion_cb(struct osmo_io_fd *iofd, int rc, const struct iofd_msghdr *msghdr, struct msgb *msg) +{ + switch (msghdr->action) { + case IOFD_ACT_WRITE: + if (iofd->io_ops.write_cb) + iofd->io_ops.write_cb(iofd, rc, msg); + break; + case IOFD_ACT_SENDTO: + if (iofd->io_ops.sendto_cb) + iofd->io_ops.sendto_cb(iofd, rc, msg, &msghdr->osa); + break; + case IOFD_ACT_SENDMSG: + if (iofd->io_ops.sendmsg_cb) + iofd->io_ops.sendmsg_cb(iofd, rc, msg); + break; + default: + OSMO_ASSERT(0); + } +} + /*! completion handler: Internal function called by osmo_io_backend after a given I/O operation has completed * \param[in] iofd I/O file-descriptor on which I/O has completed * \param[in] rc return value of the I/O operation @@ -518,18 +538,45 @@ { int idx, i;

- /* Re-enqueue the complete msgb. */ - if (rc == -EAGAIN) { - iofd_txqueue_enqueue_front(iofd, msghdr); + if (OSMO_UNLIKELY(rc < 0)) { + /* Re-enqueue the complete msgb. */ + if (rc == -EAGAIN) { + iofd_txqueue_enqueue_front(iofd, msghdr); + return; + } + + /* Propagate error for all msgbs to user cb: */ + for (idx = 0; idx < msghdr->io_len; idx++) { + iofd_send_completion_cb(iofd, rc, msghdr, msghdr->msg[idx]); + msgb_free(msghdr->msg[idx]); + msghdr->msg[idx] = NULL; + + /* The user can unregister/close the iofd during callback above. */ + if (!IOFD_FLAG_ISSET(iofd, IOFD_FLAG_FD_REGISTERED)) + break; + } + iofd_msghdr_free(msghdr); return; }

for (idx = 0; idx < msghdr->io_len; idx++) { struct msgb *msg = msghdr->msg[idx]; - int chunk; + int chunk = OSMO_MIN(rc, msgb_length(msg)); + /* chunk contains write completed bytes of msg */

- /* Incomplete write */ - if (rc > 0 && rc < msgb_length(msg)) { + /* If "rc == 0 and msgb_length(msg) > 0", then we had a partial + * write where OS wrote up to exact start point of current msg. + * In that case, we want to skip notifying the user and continue + * below on the "Incomplete write" path to delay further writing. + * If "rc == 0 and msgb_length(msg) == 0", the user specifically + * requested a write() of 0 bytes and 0 can be returned, which + * has its use cases for regular files. In that case user is + * interested in receiving a cb. */ + if (OSMO_LIKELY(!(rc == 0 && msgb_length(msg) > 0))) + iofd_send_completion_cb(iofd, chunk, msghdr, msg); + + /* Incomplete write: */ + if (chunk < msgb_length(msg)) { /* Keep msg with unsent data only. */ msgb_pull(msg, rc); msghdr->iov[idx].iov_len = msgb_length(msg); @@ -551,35 +598,12 @@ return; }

- if (rc >= 0) { - chunk = msgb_length(msg); - if (rc < chunk) - chunk = rc; - } else { - chunk = rc; - } - - /* All other failure and success cases are handled here */ - switch (msghdr->action) { - case IOFD_ACT_WRITE: - if (iofd->io_ops.write_cb) - iofd->io_ops.write_cb(iofd, chunk, msg); - break; - case IOFD_ACT_SENDTO: - if (iofd->io_ops.sendto_cb) - iofd->io_ops.sendto_cb(iofd, chunk, msg, &msghdr->osa); - break; - case IOFD_ACT_SENDMSG: - if (iofd->io_ops.sendmsg_cb) - iofd->io_ops.sendmsg_cb(iofd, chunk, msg); - break; - default: - OSMO_ASSERT(0); - } - msgb_free(msghdr->msg[idx]); msghdr->msg[idx] = NULL;

+ /* At least current msg (idx) was written, maybe more: rc >= chunk */ + rc -= chunk; + /* The user can unregister/close the iofd during callback above. */ if (!IOFD_FLAG_ISSET(iofd, IOFD_FLAG_FD_REGISTERED)) break;