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?