Attention is currently required from: osmith, Hoernchen, neels, laforge, pespin, fixeria. daniel has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30934 ) Change subject: Add osmo_io with initial poll backend ...................................................................... Patch Set 3: (33 comments) This change is ready for review. File include/osmocom/core/osmo_io.h: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a0e3d24c_0c13d2a8 PS3, Line 49: void osmo_iofd_read_enable(struct osmo_io_fd *iofd); Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/77b6da4e_53027caf PS3, Line 55: unsigned int osmo_iofd_get_priv(struct osmo_io_fd *iofd); Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/2e098e12_c9327515 PS3, Line 56: int osmo_iofd_get_fd(struct osmo_io_fd *iofd); Done File src/osmo_io.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d1fe65a8_5f61b0c1 PS3, Line 44: extern struct iofd_backend_ops iofd_poll_ops; Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9faaa528_cda404be PS3, Line 47: static enum osmo_io_backend g_backend; Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/38b482c4_bdaa1ff1 PS3, Line 85: * \returns the newly allocated msghdr or NULL in case of error */ Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d5b4285c_c62e8f3a PS3, Line 121: iofd->msgb_alloc.size + headroom, headroom, iofd->name); For io_uring the msghdr will also be used for read requests and then the msg might continue to live in the read_cb after the msghdr has already been freed. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/93a037c3_1ed3de9e PS3, Line 181: Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7f3b57f6_8f6e35ca PS3, Line 203: e It can be static https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/880fcbfd_46071b9b PS3, Line 224: superfluous Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9c49029c_dd87789b PS3, Line 225: msg_pending = iofd_msgb_alloc(iofd); This path is chosen if we receive more than one message. The original msgb is passed on to the recv callback (where it will be freed) and the pending data is stored in a new msgb. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/0e19c65a_be3c04f1 PS3, Line 230: /* Trim the original msgb to size */ pending should always be NULL here because either iofd_msgb_pending() of iofd_msgb_pending_or_alloc() has been called before. I'll see if I can restructure the code to make it clearer, like passing msgbs in and out here and setting iofd->pending in the functions that call this one. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/56f23a5c_0255cef3 PS3, Line 271: { Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/05bd9ad5_b68c6af8 PS3, Line 273: Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/2cc0a478_cf885f44 PS3, Line 306: { Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/fcfdae14_dfe16f0e PS3, Line 308: Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/36a34157_d300dbb4 PS3, Line 360: t osmo_io_fd); Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/db9e9630_ce96f7e9 PS3, Line 360: smo_io_fd); : if (!iofd I guess we could even have defaults for msgb size/headroom and have osmo_iofd_set_alloc_info() available to set it manually? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/87d6056c_515bbebe PS3, Line 391: Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/b19c7579_2ddc1353 PS3, Line 408: i Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/804141de_c1216e69 PS3, Line 410: Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d83e1f13_af9084b0 PS3, Line 416: The close callback return value indicates whether the free() should be deferred or not, it does not indicate failure. E.g. if a user calls osmo_iofd_close() in the write callback the close callback will only set a flag and the iofd will be freed at the end. Thinking about this osmo_iofd_close() should probably return the value from close() https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/04f96390_aceebb06 PS3, Line 432: >priv_nr; I also added set_data() in case some user needs to set it late or change it File src/osmo_io_internal.h: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/901202a5_2ac70407 PS3, Line 87: // TODO: SCTP_* I wanted to keep this patch as-is and add sctp support in a separate one. It's not finished yet, though. File src/osmo_io_poll.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/5ce127e1_7ba3d955 PS3, Line 40: what In socket.c it's called what in the callback, when in the ofd and flags in the internal code. I'd keep the name to match what osmo_fd users expect. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/b05225cb_040acd25 PS3, Line 63: if (iofd->closing) { Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a76fd78f_913470bb PS3, Line 75: return; There is no iofd->flags, did you mean msghdr->flags? I fact I could simply use sendmsg for everything and have wrappers around that. (write vs sendto) That might reduce duplication and we're already using struct msghdr anyway https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c7d52183_5cbefaec PS3, Line 76: } Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/391780e1_f7888181 PS3, Line 108: if (rc > 0) I'd use sizeof(saddr) since that most accurately describes the size recvfrom is allowed to write to. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/22b09dc2_e73db721 PS3, Line 114: Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7609bae7_46d28400 PS3, Line 117: { Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/06ac3651_6dd70aa7 PS3, Line 124: struct osmo_sockaddr *dest = &msghdr->sa; Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/1054c3ea_055e6adb PS3, Line 146: i Done -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30934 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I50d73cf550d6ce8154bf827bf47408131cf5b0a0 Gerrit-Change-Number: 30934 Gerrit-PatchSet: 3 Gerrit-Owner: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: Hoernchen <ewild(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: osmith <osmith(a)sysmocom.de> Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Fri, 20 Jan 2023 13:52:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith <osmith(a)sysmocom.de> Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment