Attention is currently required from: arehbein, pespin.
Patch set 20:Code-Review -2
The change is no longer submittable: Code-Review is unsatisfied now.
4 comments:
File include/osmocom/netif/ipa.h:
Patch Set #20, Line 6: #include <osmocom/netif/stream.h>
This header file contains a protocol definition, and making it contain the whole stream API does not look right to me. AFAIU, this header needed for `struct osmo_stream_srv` pointers? If so, you could just add a forward declaration instead of including the whole header.
File src/ipa.c:
Patch Set #20, Line 443: struct msgb *msg
If we go for exposing this API (see my other comment), the `msg` pointer should become the first argument for the sake of consistency with other API.
Patch Set #20, Line 456: osmo_ipa_stream_srv_send
TBH, I don't really see the benefits of having this API. It's just three lines of code (or even two, if we exclude the assert) in both cases. Where exactly and how much this API is going to be used?
My concern is that neither it's purely a `stream_{srv,cli}` API, nor purely an IPA specific API - it's a mix of both. And it's not like you're saving a lot of effort by using it.
As a compromise, I would suggest adding only the `[osmo_]ipa_push_headers()` function, but not `osmo_ipa_stream_{srv,cli}_send()`. Below is how a typical use of this API would look like:
```
osmo_ipa_push_headers(msg, IPAC_PROTO_OSMO, IPAC_PROTO_EXT_CTRL);
osmo_stream_{srv, cli}_send({conn, cli}, msg);
```
File tests/stream/stream_test.c:
Patch Set #14, Line 582: osmo_ipa_stream_srv_send(conn, IPAC_PROTO_IPACCESS, -1, m);
I still think mixing filling the msgb (just 1-2 API calls) + sending is a bad idea, but since others […]
Agreeing with @pespin@sysmocom.de here.
To view, visit change 33201. To unsubscribe, or for help writing mail filters, visit settings.