Change in libosmocore[master]: Add osmo_io with initial poll backend

Attention is currently required from: osmith, Hoernchen, neels, laforge, fixeria, daniel. pespin 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: (26 comments) File include/osmocom/core/osmo_io.h: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/8d04803d_26ccd6a7 PS3, Line 49: void osmo_iofd_read_enable(struct osmo_io_fd *iofd); duplicated declaration? one probably should be "disable" https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a893cd75_7d281ec1 PS3, Line 55: unsigned int osmo_iofd_get_priv(struct osmo_io_fd *iofd); const iofd https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9779c049_4723f660 PS3, Line 56: int osmo_iofd_get_fd(struct osmo_io_fd *iofd); const iofd File src/osmo_io.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4eb23142_d4708c38 PS3, Line 44: extern struct iofd_backend_ops iofd_poll_ops; we have an internal header, maybe move this there? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/b45183be_4c0b39cd PS3, Line 47: static enum osmo_io_backend g_backend; g_io_backend? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/325f6a28_d6e94902 PS3, Line 50: struct iofd_backend_ops g_iofd_ops; then better name it with an osmo_ prefix. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/5f694a6e_7c527ff6 PS3, Line 111: talloc_free(msghdr); the ownserhip of msghdr->msg is not really clear here. If a msg ptr is passed in iofd_msghdr_alloc, is it expected to pass ownsership of msg to msghdr? If so, either: - call talloc_steal in iofd_msghdr_alloc - explicitly talloc_free msghdr->msg here. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c5f10141_edbf357f PS3, Line 203: e Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/3989be04_0f4f8bb4 PS3, Line 225: msg_pending = iofd_msgb_alloc(iofd); I wonder why do we need to allocate + copy to a new msgb here. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d9474b17_ea77ad96 PS3, Line 228: iofd->pending = msg_pending; iofd->pending may already be set here? in that case may leak the previous iofd->pending? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9cf86edf_f55914c5 PS3, Line 299: int osmo_iofd_sendto_msgb(struct osmo_io_fd *iofd, struct msgb *msg, const struct osmo_sockaddr *dest) we may be missing "int flags" here? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/8d0ed2a8_3583faa5 PS3, Line 367: iofd->name = talloc_strdup(iofd, name); This can be split into a separate func osmo_iofd_set_name(), simplifying the current osmo_iofd_setup param list. Some apps may not even care nor want to spend cycles allocating memory and setting names. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9dd628e7_0023c4fa PS3, Line 389: g_backend == OSMO_IO_BACKEND_POLL ? "poll" : "uring" Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f517d93a_a320919a PS3, Line 408: if (iofd->pending) no need for this if, must probably (free function) https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/5e7e43dd_5e60bd63 PS3, Line 414: talloc_free(iofd); you still want to free the memory if close somehow fails, otherwise you'll leak it? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7decfc04_2f211349 PS3, Line 430: osmo_iofd_get_priv Ack File src/osmo_io_internal.h: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f501f63f_e7dff15c PS3, Line 72: struct { : bool read_enabled; : bool read_pending; : bool write_pending; : bool write_enabled; : /* TODO: index into array of registered fd's? */ : } uring; Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7ff40994_73fcb0ba PS3, Line 87: // TODO: SCTP_* I'd say this needs to be added before merging the patch? or not needed? File src/osmo_io_poll.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c4b0bb76_56537732 PS3, Line 63: if (iofd->closing) { Remove {} https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4f2e3acc_e61c1809 PS3, Line 71: rc = write(ofd->fd, msgb_data(msg), msgb_length(msg)); this should be send() in order to pass iofd->flags to it. Actually, you should have aseparate read_flags and write_flags fields most probably. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/ef75ef5d_767d6bba PS3, Line 72: if (rc < msgb_length(msg)) { if write returns error (rc<0), you'll be doing pretty weird stuff here. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/2c843641_2b92acf7 PS3, Line 104: socklen_t addrlen = sizeof(struct sockaddr); This is wrong afaict. It should be struct sockaddr_strorage. No need for sa, You can simply do: socklen_t addrlen = sizeof(saddr.u.ss); rc = recvfrom(ofd->fd, msgb_data(msg), msgb_tailroom(msg), 0, &saddr.u.sa, &addrlen); https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/0b2e6e3a_49d7b8f0 PS3, Line 114: Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4b1467a4_feb2bfb2 PS3, Line 117: { Ack https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/f6938d24_856ee183 PS3, Line 124: struct osmo_sockaddr *dest = &msghdr->sa; Usually we use "osa" to name an osmo_sockaddr, to differentiate it from "sa" (sockaddr). Please update the field name in msghdr. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/fa350aec_7561a9b7 PS3, Line 128: &dest->u.sa, sizeof(*dest)); not sure if sizeof(sockaddr_storage) is good here, you may have to pass dest.u.sin or dest.u.sin6 depending on the dest.u.sa.sa_family? -- 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 &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Reviewer: Hoernchen &lt;ewild(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Attention: Hoernchen &lt;ewild(a)sysmocom.de&gt; Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Comment-Date: Tue, 17 Jan 2023 12:12:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-MessageType: comment