Bug #6437
openosmo_io_ops segmentation_cb missing parameter holding state
90%
Description
struct osmo_io_ops { ... void (*read_cb)(struct osmo_io_fd *iofd, int res, struct msgb *msg); ... void (*write_cb)(struct osmo_io_fd *iofd, int res, struct msgb *msg); ... int (*segmentation_cb)(struct msgb *msg);
The segmentation_cb is missing the "struct osmo_io_fd *iofd" in front. This puts several limitations for future users of the cb API, since there's no way to access related objects like iofd (private_nr, data ptr, etc.), which in turn doesn't allow higher level APIs (such as osmo_stream) to provide its own wrapper API passing eg. osmo_stream_cli/srv as a first parameter.
That means there's no way to store inter-msg state which may be needed at some point for segmentation, which doesn't come initially from first msgb.
segmentation_cb was added in libosmocore d4d03705f90e4a346f21ee2f328348b9ba37a201, between release 1.8.0 and 1.9.0.
Relevant users of segmentation_cb outside libosmocore which could be affected are:
libosmo-netif.git:
libosmo-netif/src/stream_srv.c 977: * \param[in] segmentation_cb Segmentation callback to be set */ 978:void osmo_stream_srv_set_segmentation_cb(struct osmo_stream_srv *conn, 979: osmo_stream_srv_segmentation_cb_t segmentation_cb) 988: conn_ops.segmentation_cb = segmentation_cb; libosmo-netif/src/stream_cli.c 110: osmo_stream_cli_segmentation_cb_t segmentation_cb; 433: cli->segmentation_cb = NULL; 490: .segmentation_cb = NULL, 527: .segmentation_cb = NULL, 663:static void configure_cli_segmentation_cb(struct osmo_stream_cli *cli, 664: osmo_stream_cli_segmentation_cb_t segmentation_cb) 670: client_ops.segmentation_cb = segmentation_cb; 676: * \param[in] segmentation_cb Target segmentation callback 678:void osmo_stream_cli_set_segmentation_cb(struct osmo_stream_cli *cli, 679: osmo_stream_cli_segmentation_cb_t segmentation_cb) 681: cli->segmentation_cb = segmentation_cb; libosmo-netif/src/ipa.c 411:int osmo_ipa_segmentation_cb(struct msgb *msg) libosmo-netif/include/osmocom/netif/ipa.h 66:int osmo_ipa_segmentation_cb(struct msgb *msg);
libosmo-sccp.git:
libosmo-sccp/src/ss7_internal.h 41:int xua_tcp_segmentation_cb(struct msgb *msg); libosmo-sccp/src/osmo_ss7_xua_srv.c 89: osmo_stream_srv_set_segmentation_cb(srv, osmo_ipa_segmentation_cb); 96: osmo_stream_srv_set_segmentation_cb(srv, xua_tcp_segmentation_cb); 103: osmo_stream_srv_set_segmentation_cb(srv, NULL); libosmo-sccp/src/osmo_ss7_asp.c 652: osmo_stream_cli_set_segmentation_cb(asp->client, osmo_ipa_segmentation_cb); 657: osmo_stream_cli_set_segmentation_cb(asp->client, NULL); 660: osmo_stream_cli_set_segmentation_cb(asp->client, xua_tcp_segmentation_cb); 667: osmo_stream_cli_set_segmentation_cb(asp->client, NULL); 856:int xua_tcp_segmentation_cb(struct msgb *msg)
osmo-bsc.git:
osmo-bsc/src/osmo-bsc/cbsp_link.c 109: osmo_stream_srv_set_segmentation_cb(srv, osmo_cbsp_segmentation_cb); 231: osmo_stream_cli_set_segmentation_cb(cbc->client.cli, osmo_cbsp_segmentation_cb);
Updated by pespin about 1 month ago
libosmo-netif started using that cb in 0e7028f742dedefd9fccc2d5b26875fca4036f58, which means between 1.3.0 and 1.4.0 release, which means there's already released code using the old callback signature.
It's probably the same stuff for libosmo-sccp and osmo-bsc.
Updated by pespin about 1 month ago
We are anyway already breaking osmo_io_ops ABI with next upcoming release of libosmocore in several ways, eg from libosmocore.git TODO-RELEASE:
"core ABI change osmo_io_ops now contains a struct of structs, not union of structs"
So I think it makes sense to change the callback now anyway.
Alternatively, we could keep the old segmentation_cb and add a segmentation_cb2 field with the new signature, and osmo_io code calls whichever of the 2 is available, giving preference to the new one.
Updated by pespin about 1 month ago
- Status changed from New to Feedback
RFC patch submitted here: https://gerrit.osmocom.org/c/libosmocore/+/36582 osmo_io: Add iofd param to segmentation_cb
We can also change it to have a new segmentation_cb2 callback to avoid breaking API (still breaking ABI but we are anyway already breaking it regardless of merging this).
Updated by laforge about 1 month ago
pespin wrote in #note-3:
We can also change it to have a new segmentation_cb2 callback to avoid breaking API (still breaking ABI but we are anyway already breaking it regardless of merging this).
I would prefer that.
Updated by pespin about 1 month ago
- Assignee set to pespin
- % Done changed from 0 to 90