Attention is currently required from: laforge, pespin, daniel.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33201 )
Change subject: stream: Add and use IPA send function ......................................................................
Patch Set 13:
(10 comments)
File examples/ipa-stream-client.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/39ea5790_a03fe733 PS11, Line 109: if (osmo_ipa_process_msg(msg) < 0) {
what about this comment?
Done
File examples/ipa-stream-server.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/1c6bc327_d66ed1a3 PS11, Line 54: if (osmo_ipa_process_msg(msg) < 0) {
Remove this, it's now handled in the segmentation_cb
This had previously been done separately in change I53283ec7bd7f07dfa612681ae84af93d5cd098b9 . I have adapted the patch series now since your uploads incorporated removing this in previous changes of the series.
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/2a224216_0c1d18bc PS11, Line 60: osmo_ipa_stream_srv_send(conn, IPAC_PROTO_UNSPECIFIED, IPAC_PROTO_UNSPECIFIED, msg);
Using the proto/proto_ext getters here is more clear imho than using IPAC_PROTO_UNSPECIFIED and deal […]
I've changed it and I couldn't agree more.
File include/osmocom/netif/ipa.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/63e37189_6e441808 PS12, Line 57: #define IPAC_PROTO_UNSPECIFIED -1
it's a bit strange to declare this here while others are probably declared somewhere else?
Had renamed this to `OSMO_IPA_IPAC_PROTO_UNSPECIFIED` (the 'other' values are in `libosmocore.git:src/gsm/protocoll/ipaccess.h`; assumed you wouldn't have wanted a non-protocol value in there), but then removed the functionality using it due to another review comment.
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/aecfd2d5_12e68ed7 PS10, Line 475: void ipa_stream_srv_send
ah yes I wondered about this shortly... […]
Done
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/b48a6481_d44cf902 PS11, Line 481: if (ipaccess_proto == IPAC_PROTO_UNSPECIFIED) { : ipaccess_proto = msg_get_ipa_proto(msg); : pe = msg_get_ipa_proto_ext(msg); : }
I wouldn't special-case this here. […]
I assume by that you mean you don't want this functionality anymore, altogether (otherwise I have no idea where else this check should go). I removed it.
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/fb220732_d14da066 PS1, Line 1959: OSMO_ASSERT
Yes, because `conn` isn't used by this function before it is passed to `osmo_stream_srv_send()`, whi […]
Done
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/2f907762_8cb5c868 PS5, Line 1947: /* msgb_l1(msg) is expected to be set */
The idea was providing the possibility of quickly reusing the headers of a message received in IPA m […]
Done
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/49e009ae_c9754cda PS11, Line 47: #include <osmocom/netif/ipa.h>
Unneeded
Done
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/4545f5c3_3d7f354b PS11, Line 612: if (res == 0) {
Unrelated/Unneeded?
Done
-
arehbein