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
--
To view, visit
https://gerrit.osmocom.org/c/libosmo-netif/+/33201
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I61e1fe59166c46595efe8c1f32b8f2607cb6c529
Gerrit-Change-Number: 33201
Gerrit-PatchSet: 13
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 29 Jul 2023 18:58:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: comment