Project

General

Profile

Actions

Bug #6437

open

osmo_io_ops segmentation_cb missing parameter holding state

Added by pespin 13 days ago. Updated 11 days ago.

Status:
Feedback
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
04/17/2024
Due date:
% Done:

90%

Spec Reference:

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);

Actions #1

Updated by pespin 13 days 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.

Actions #2

Updated by pespin 13 days 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.

Actions #3

Updated by pespin 13 days 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).

Actions #4

Updated by laforge 11 days 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.

Actions #5

Updated by pespin 11 days ago

  • Assignee set to pespin
  • % Done changed from 0 to 90

laforge wrote in #note-4:

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.

Submitted new version of the patch accordingly.

Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)