Attention is currently required from: pespin.
6 comments:
Patchset:
@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:
Patch Set #1, 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
Fixes: df1ee8568b97dbf6d5268a83d1715a1c1fffb2de
OK, I'll add this notation in the next iteration.
File src/core/osmo_io.c:
Patch Set #1, 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.
Patch Set #1, 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.
Patch Set #1, Line 761: } else {
if clause is an early return, this "else" can be removed and the assignment done directly.
Acknowledged
To view, visit change 39276. To unsubscribe, or for help writing mail filters, visit settings.