Attention is currently required from: daniel. laforge 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: (8 comments) Patchset: PS3: In general I think we need to think about naming / namespace pollution. I guess the iofd_* functions are non-static as they will end up geting used form the io_uring backend in another file? Unfortunately, for historical reasons, libosmocore doesn't have a .map file so all the non-static symbols become publicly accessible to applications. And the general rule for (new) library-exported symbols is to use the osmo_ prefix. So I think the cleanest approach would actually be to introduce a .map file and then only export the osmo_iofd_* functions that way, while keeping the iofd_* non-static but not exported? File src/osmo_io.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/82a7173e_92c7c48e PS3, Line 157: */ not critical: I usually put the end-of-comment on the previous line to save extraneous lines in the file (more context on the screen) and to optically have the doxygen-docs directly above the function definition, without a (mostly) empty line. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/6f6a2141_d40e53cb PS3, Line 203: e no doxygen docs, but function is exported https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d3f6a27e_69281393 PS3, Line 224: superfluous superflous IMHO means that it's not needed. Maybe "extraneous"? "leftover"? "pending"? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d3baccb7_7397c510 PS3, Line 271: { coding style https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/7903a154_ac2309c6 PS3, Line 306: { coding style https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/430479e2_d7e320ad PS3, Line 356: unsigned int size, unsigned int headroom, : int fd we have three integer argiuments after each other, and lots of arguments, which makes it easy to have bugs on the caller side. Maybe we should re-order them somehow (or reduce, if possible)? Logically I would say that "ctx, fd" should be the two first arguments, as we usually have the object the call relates to/on as the first argument, prefixed by th ctx. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d5e0682b_122c0ff0 PS3, Line 358: unsigned int priv_nr given that there's only one known legacy user of the priv_nr (libosmo-abis), maybe we remove this from the function prototype fore new API. Those few callers that actually care can still set it in the io_fd that is returned from this function? -- 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: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Tue, 17 Jan 2023 11:15:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment