Attention is currently required from: pespin.

falconia has posted comments on this change by falconia. ( https://gerrit.osmocom.org/c/libosmocore/+/39276?usp=email )

Change subject: osmo_iofd_register: fix the case of changing fd ......................................................................

Patch Set 1:

(6 comments)

Patchset:

PS1:

@falcon@freecalypso. […]

I am working on OS#6474. I got this code:

https://gitea.osmocom.org/themwi/twrtp-proto

which I am currently reworking into a series of patches for libosmo-netif, with the goal of bringing my new RTP implementation into Osmocom proper and thus making it available to OsmoBTS and others in the future. The version which I plan to submit as patches to libosmo-netif will be a little different from the just-linked external/standalone version: aside from renaming headers and APIs to fit into Osmocom namespace, I am reworking the core structures (`struct twrtp_jibuf_inst` which will become `struct osmo_twjit` and `struct twrtp_endp` which will become `struct osmo_twrtp`) to be opaque instead of fully exposed - hence I need to provide APIs for all explicitly-allowed external operations.

In the new version there will be an API that takes in OS-level fds for RTP and RTCP sockets and feeds them to `osmo_iofd_register()` on `osmo_io_fd` instances that are internal to the opaque twrtp instance. The problem is - what happens if one of those `osmo_iofd_register()` operations fails? My twrtp API needs to fail cleanly, and leave the twrtp instance in a state where the user could try again and potentially succeed next time. I reviewed osmo_io code to see if it does what I expect, and noticed that if `osmo_iofd_register()` fails, the failed fd will remain stored, and the next `osmo_iofd_register()` attempt will ignore the fd passed to it.

Commit Message:

https://gerrit.osmocom.org/c/libosmocore/+/39276/comment/0926db90_88079472?u... : PS1, Line 13: osmo_fd_setup(), or if it has changed since then, you can state

"or if it has changed since then" I probably missed that part when I caused the regression recently, […]

Acknowledged

https://gerrit.osmocom.org/c/libosmocore/+/39276/comment/96f838a1_3c01affc?u... : PS1, Line 22:

Fixes: df1ee8568b97dbf6d5268a83d1715a1c1fffb2de

OK, I'll add this notation in the next iteration.

File src/core/osmo_io.c:

https://gerrit.osmocom.org/c/libosmocore/+/39276/comment/5f8b3498_bb0eb7e5?u... : PS1, Line 756: else if (iofd->fd < 0)

I think you need to leave this as is. […]

Right after the `else if` clause I removed, the code checks `IOFD_FLAG_FD_REGISTERED`. How can `iofd->fd` be -1 if the registered flag is set? And if the registered flag is not set, then my code sets `iofd->fd` to the new fd unconditionally.

But maybe I'll leave this code unchanged in the next iteration - let me think about it.

https://gerrit.osmocom.org/c/libosmocore/+/39276/comment/e7c591f3_e3a32f94?u... : PS1, Line 760: return iofd->fd == fd ? 0 : -ENOTSUP;

since you are not setting iofd->fd in the code path you removed, this check is also wrong now?

See my other comment above.

https://gerrit.osmocom.org/c/libosmocore/+/39276/comment/7e3efd03_e4238e8b?u... : PS1, Line 761: } else {

if clause is an early return, this "else" can be removed and the assignment done directly.

Acknowledged