Attention is currently required from: arehbein, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33201?usp=email )
The change is no longer submittable: Code-Review is unsatisfied now.
Change subject: stream: Add and use IPA send function ......................................................................
Patch Set 20: Code-Review-2
(4 comments)
File include/osmocom/netif/ipa.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/d05bd983_5908bc94 PS20, 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:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/2090a5d5_b859887f PS20, 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.
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/929bbed7_f908aea5 PS20, 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:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/b35d9c86_6db1628c PS14, 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.
-
fixeria