Bug #6263
openimpossible to detect wrong callbacks being registered / osmo_io_ops union
40%
Description
As I just had to learn the hard way, it's possible to create an osmo_io_fd using osmo_iofd_setup in one mode (e.g. OSMO_IO_FD_MODE_READ_WRITE) but the register an osmo_io_ops with wrong call-backs (e.g. recvfrom/sendto). Given that it's a union, there's nothing we can do at runtime to detect such an error and ASSERT or the like.
We'd either have to break ABI by introducing a "enum osmo_io_fd_mode" member in the struct, or we'd have to expand all those unions into a struct. This way we can see which function pointers are NULL and non-NULL and hence determine if the right call-backs for the given mode were set.
Any ideas/comments?
Updated by daniel 5 months ago
With the additional mode enum we could still get a struct that has mode set to send/recv but sets sendto/recvfrom in the union.
I'd tend to remove the union at least on the public io_ops enum and assert that the correct function pointers are non-NULL. We could keep the union inside osmo_io_fd if we want to keep the space savings there.
But wouldn't that also be ABI breakage since the function pointers are now at different locations?
Another idea would be to have different functions for each mode
osmo_iofd_set_sndrecv() osmo_iofd_set_sndtorecvfrom() ...
These would take the relevant function pointers directly and return an error/assert if the iofd is not in the correct mode.
Updated by laforge 5 months ago
On Mon, Nov 20, 2023 at 12:58:07PM +0000, daniel wrote:
I'd tend to remove the union at least on the public io_ops enum and assert that the correct function pointers are non-NULL.
agreed.
We could keep the union inside osmo_io_fd if we want to keep the space savings there.
I don't think we need to care about 2-4 pointers.
But wouldn't that also be ABI breakage since the function pointers are now at different locations?
Yes, it is ABI breakage, so we have to mark it in TOOD-RELEASE. It can be done api-compatible if the member
names don't change.
Updated by daniel 5 months ago
- Status changed from Feedback to In Progress
- % Done changed from 0 to 20
laforge wrote in #note-2:
On Mon, Nov 20, 2023 at 12:58:07PM +0000, daniel wrote:
We could keep the union inside osmo_io_fd if we want to keep the space savings there.
I don't think we need to care about 2-4 pointers.
Ok
But wouldn't that also be ABI breakage since the function pointers are now at different locations?
Yes, it is ABI breakage, so we have to mark it in TOOD-RELEASE. It can be done api-compatible if the member
names don't change.
ACK
