Bug #6393
openosmo_io conflating osmo_iofd_unregister vs osmo_iofd_close
0%
Description
I've been starting to work on something like an osmo_io API user manual, and as part of that tried to think about how to guard osmo_io against invalid usage.
We have- osmo_iofd_unregister
- osmo_iofd_close
- osmo_iofd_free
- osmo_iofd_unregister sets the IOFD_FLAG_CLOSED flag, even though it does not close
- osmo_iofd_register clears the IOFD_FLAG_CLOSED, so there's some symmetry, despite the name
- osmo_iofd_close also sets the IOFD_FLAG_CLOSED flag, but without unregistering
- this mean we cannot use the IOFD_FLAG_CLOSED to check in osmo_iofd_unregister if the iofd was already unregistered
- it actually doesn't close the fd unless there is an osmo_iofd_ops.close call-back from the backend
- however, it does always set iofd->fd to -1, so if no such call-back exists, we have a fd leak
- osmo_iofd_free calls osmo_iofd_close, but not osmo_iofd_unregister. So it somehow tries to do whatever is needed before the free, but only half of it.
I'm not entirely sure yet how to untangle this. Maybe we need multiple flags to properly distinguish the closed from the unregistered state? Or maybe we don't need another flag and use iofd->fd == -1 to determine the actual closed status, and rename the CLOSED flag to n UNREGISTERED flag?
Updated by laforge about 2 months ago
- if we are using the osmo_fd back-end, osmo_iofd_close will call the iofd_poll_close, which calls iofd_poll_unregister, which in turn will actually unregister the osmo_fd from the internal structures.
- the poll backend internally uses the IOFD_FLAG_FD_REGISTERED to guard against repeat register or unregister
- if we are using the io_uring back-end, osmo_iofd_close will call the iofd_uring_close, which in turn calls iofd_uring_unregister, which will unregister the io_fd from the internal structures
- the io_fd backend internally does not use a flag to guard against repeat register or unregister.
So despite osmo_iofd_close() not calling osmo_iofd_unregister(), the back-ends interenally will actually perform an unregister. Maybe we should pull that up to the generic osmo_io layer to make it more obvious and explicit?
What remains is the problem that the CLOSED flag actually doesn't differentiate between unregister and close.
Updated by laforge about 2 months ago
laforge wrote:
Oddities:
- osmo_iofd_unregister sets the IOFD_FLAG_CLOSED flag, even though it does not close
- osmo_iofd_register clears the IOFD_FLAG_CLOSED, so there's some symmetry, despite the name
Well, actually the first sentence is true only for a hypothetical back-end that doesn't provide an unregister_cb. All the real-world back-ends do have that unregister_cb and hence the code path setting IOFD_FLAG_CLOSED in the generic code isn't actually taken.
So we have the situation:- osmo_iofd_setup and osmo_iofd_close set the IOFD_FLAG_CLOSED flag
- osmo_iofd_register unsets the IOFD_FLAG_CLOSED
However, when we unregister, the flag does not get set, and hence we cannot use that flag to guard us against invalid repeat invocations of osmo_iofd_register(). My head starts spinning...
