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: (10 comments) File src/osmo_io.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/9bae02a3_ce85f35f PS3, Line 389: g_backend == OSMO_IO_BACKEND_POLL ? "poll" : "uring" there might be more in the future, it might be worth having a separate value_string for that rather than explicitly hard-coding the names on the caller side here. Also, it doesn't hurt to log the actaul fd number while we're logging something anyway. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/a750a7b0_dc9a555c PS3, Line 406: / c++ style comment https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/c23a484a_4615b9f0 PS3, Line 430: osmo_iofd_get_priv please rename to osmo_iofd_get_priv_nr(). "priv" is also sometimes used as alias to "data" in kernel and osmocom. If we remove the argument from the setup function, then we also need a set_priv_nr(). File src/osmo_io_internal.h: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/221f9c34_279a479c PS3, Line 17: in I'm wondering if it had any advantage to have only a single function with an argument stating the "operation"? The prototype is almost identical (just int/void return). I'm not sure it would have any advantage. Probably none. https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/4851bdb7_ac9ffd4a 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; should this maybe introduced only in the actual io_oring patch? File src/osmo_io_poll.c: https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/620c95b7_ad90f478 PS3, Line 84: / maybe still look into this before merging it? https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/26ed1231_29e5d8c9 PS3, Line 114: extraneous empty line https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/85d8716d_adce74ab PS3, Line 117: { coding style https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/d4b0e7dc_8388e60e PS3, Line 143: / same as above https://gerrit.osmocom.org/c/libosmocore/+/30934/comment/13c1983d_94d3470c PS3, Line 146: i should be static? same as all of the iofd_poll_ functions which are exposed as callback via iofd_poll_ops and don't need to be exported as global functions -- 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:24:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment