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); > duplicated declaration? one probably should be "disable" 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); > const 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); > const 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; > we have an internal header, maybe move this there? Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9faaa528_cda404be PS3, Line 47: static enum osmo_io_backend g_backend; > g_io_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 */ > I suggest printing an error message and exit(1) here, since this may be the > result of an unexpected […] Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d5b4285c_c62e8f3a PS3, Line 121: iofd->msgb_alloc.size + headroom, headroom, iofd->name); > the ownserhip of msghdr->msg is not really clear here. […] 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: > always Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7f3b57f6_8f6e35ca PS3, Line 203: e > Ack It can be static https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/880fcbfd_46071b9b PS3, Line 224: superfluous > superflous IMHO means that it's not needed. […] Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9c49029c_dd87789b PS3, Line 225: msg_pending = iofd_msgb_alloc(iofd); > I wonder why do we need to allocate + copy to a new msgb here. 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 */ > iofd->pending may already be set here? in that case may leak the previous > iofd->pending? 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: { > coding style Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/05bd9ad5_b68c6af8 PS3, Line 273: > coding style Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/2cc0a478_cf885f44 PS3, Line 306: { > coding style Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/fcfdae14_dfe16f0e PS3, Line 308: > coding style Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/36a34157_d300dbb4 PS3, Line 360: t osmo_io_fd); > given that there's only one known legacy user of the priv_nr (libosmo-abis), > maybe we remove this fr […] Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/db9e9630_ce96f7e9 PS3, Line 360: smo_io_fd); : if (!iofd > we have three integer argiuments after each other, and lots of arguments, > which makes it easy to hav […] 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: > Ack Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/b19c7579_2ddc1353 PS3, Line 408: i > c++ style comment Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/804141de_c1216e69 PS3, Line 410: > no need for this if, must probably (free function) Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d83e1f13_af9084b0 PS3, Line 416: > you still want to free the memory if close somehow fails, otherwise you'll > leak it? 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; > Ack 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'd say this needs to be added before merging the patch? or not needed? 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 > might be just me, but I find "flags" much more descriptive than "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) { > Remove {} Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a76fd78f_913470bb PS3, Line 75: return; > this should be send() in order to pass iofd->flags to it. […] 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: } > if write returns error (rc<0), you'll be doing pretty weird stuff here. Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/391780e1_f7888181 PS3, Line 108: if (rc > 0) > This is wrong afaict. It should be struct sockaddr_strorage. […] 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: > extraneous empty line Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7609bae7_46d28400 PS3, Line 117: { > coding style Done https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/06ac3651_6dd70aa7 PS3, Line 124: struct osmo_sockaddr *dest = &msghdr->sa; > Usually we use "osa" to name an osmo_sockaddr, to differentiate it from "sa" > (sockaddr). […] Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/1054c3ea_055e6adb PS3, Line 146: i > should be static? same as all of the iofd_poll_ functions which are exposed > as callback via iofd_pol […] 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 <dwillm...@sysmocom.de> Gerrit-Reviewer: Hoernchen <ew...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanits...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-Attention: osmith <osm...@sysmocom.de> Gerrit-Attention: Hoernchen <ew...@sysmocom.de> Gerrit-Attention: neels <nhofm...@sysmocom.de> Gerrit-Attention: laforge <lafo...@osmocom.org> Gerrit-Attention: pespin <pes...@sysmocom.de> Gerrit-Attention: fixeria <vyanits...@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 <osm...@sysmocom.de> Comment-In-Reply-To: laforge <lafo...@osmocom.org> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
