Attention is currently required from: laforge, pespin, daniel.
Hello Jenkins Builder, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-netif/+/33197
to look at the new patch set (#13).
Change subject: stream: Add server-side (segmentation) support for IPA
......................................................................
stream: Add server-side (segmentation) support for IPA
With this commit, IPA segmentation can be taken care of by setting
the segmentation callback osmo_ipa_segmentation_cb().
Depends: libosmocore.git I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f
Related: OS#5753, OS#5751
Change-Id: I6c91ff385cb5f36ab6b6c96d0e44997995d0d24c
---
M examples/ipa-stream-server.c
M include/osmocom/netif/stream.h
M src/stream.c
M tests/stream/stream_test.c
M tests/stream/stream_test.ok
5 files changed, 381 insertions(+), 10 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/97/33197/13
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33197
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I6c91ff385cb5f36ab6b6c96d0e44997995d0d24c
Gerrit-Change-Number: 33197
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-MessageType: newpatchset
Attention is currently required from: laforge, pespin, daniel.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33197 )
Change subject: stream: Add server-side (segmentation) support for IPA
......................................................................
Patch Set 12:
(1 comment)
Patchset:
PS12:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comments/ac923843_f71a95…
No idea how this slipped through the linter again (I have the pre-commit hook installed)
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33197
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I6c91ff385cb5f36ab6b6c96d0e44997995d0d24c
Gerrit-Change-Number: 33197
Gerrit-PatchSet: 12
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 19:01:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
arehbein has abandoned this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33200 )
Change subject: examples: Add osmo_io based segmentation
......................................................................
Abandoned
The decision has been made to not have this as a separate change
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33200
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ibf5fd3a606da7888f23e6f75cc30fe433897a97b
Gerrit-Change-Number: 33200
Gerrit-PatchSet: 11
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: abandon
Attention is currently required from: laforge, pespin, daniel.
Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33197 )
Change subject: stream: Add server-side (segmentation) support for IPA
......................................................................
Patch Set 12:
(1 comment)
File tests/stream/stream_test.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-10226):
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/ac923843_f71a95fa
PS12, Line 641: }
void function return statements are not generally useful
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33197
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I6c91ff385cb5f36ab6b6c96d0e44997995d0d24c
Gerrit-Change-Number: 33197
Gerrit-PatchSet: 12
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:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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
Attention is currently required from: laforge, pespin, daniel.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33197 )
Change subject: stream: Add server-side (segmentation) support for IPA
......................................................................
Patch Set 12:
(8 comments)
File include/osmocom/netif/ipa.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/adea8e8a_d0c95623
PS10, Line 6: #include <osmocom/netif/stream.h>
> Not sure why ipa. […]
Done
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/c73a9db4_aa6ca599
PS10, Line 46: int osmo_ipa_segmentation_cb(struct msgb *msg);
> This belongs in the previous commit
Done
File include/osmocom/netif/stream.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/94d3c33b_b4c08386
PS1, Line 28: /* TODO: Add protocols for
> Done. […]
Done
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/0f040a9f_fca3c3e7
PS10, Line 39: #include <osmocom/gsm/ipa.h>
> Not needed.
Done
File tests/stream/stream_test.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/51aa40c5_f016f6c5
PS10, Line 540: osmo_stream_cli_set_addr(osc, "127.0.0.11");
> better use 127.0.0.1 here, all IP addresses being available in linux specific iirc.
I don't understand the second part of the sentence...
I copied this from the existing test code for the other tests. Should that be changed as well then? Or can we just leave it at `127.0.0.11`?
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/74c8fd4e_21215fc2
PS10, Line 621: rc = clock_gettime(CLOCK_MONOTONIC, &start);
> usually alarm() is used for this kind of stuff (exit process on timeout).
What is the advantage of using `alarm()` instead, and how wouldn't I need to register some sort of (additional, if it's already handled in lower layers) alarm handling for SIGALRM if I wanted a diagnostic message?
@daniel I prefer setting an amount of time to setting an amount of iterations, but maybe that's just me
I'd like to leave it as is if possible. Did try it out and it worked...
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/b200fbbb_020e5415
PS10, Line 629: rc = clock_gettime(CLOCK_MONOTONIC, &now);
> osmo_clock_gettime. […]
(see Daniel's reply above)
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/21a51fea_fe787c15
PS10, Line 684: rc = test_segm_ipa_stream_srv_run(tall_test, host, port, srv);
> we don't usually do this kind of return in tests. […]
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33197
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I6c91ff385cb5f36ab6b6c96d0e44997995d0d24c
Gerrit-Change-Number: 33197
Gerrit-PatchSet: 12
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:57:43 +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
Attention is currently required from: laforge, pespin, daniel.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33652 )
Change subject: ipa: Add segmentation callback
......................................................................
Patch Set 5:
(3 comments)
File include/osmocom/netif/ipa.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33652/comment/6a49898d_bb42ab28
PS4, Line 27: #define msgb_ipa_proto(__x) OSMO_MSGB_IPA_CB(__x)->proto
> osmo_msgb_ipa_cb_proto […]
I put `osmo_ipa_msgb_cb*`, because it's in `ipa.h` and not in `msgb.h` and because to me it seems less confusing to stick to filename prefixes.
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33652/comment/80f13ef4_74d76a85
PS2, Line 378: MSG_CB_IPA_PROTO_EXT_IS_SET_OFFSET
> We wouldn't need the IS_SET flag anyway, I'd expect proto_ext to only have a meaning if proto is IPA […]
Concerning the idea of a struct, I deliberately did not go for a struct in order to avoid violating the strict aliasing rule, since `msg->cb` is an array of `unsigned long`. Seems like we don't care/looks like we use `-fno-strict-aliasing` everywhere anyways, so I'll keep that in mind. At least now I know where we stand on this
https://gerrit.osmocom.org/c/libosmo-netif/+/33652/comment/639e4653_5142065a
PS2, Line 432: /* Below: Helper functions for addition of send_ipa functionality in later commit */
> Iirc the idea was to introduce ipa_check_pull_headers() AND use it in the segmentation_cb(). […]
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33652
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I87ef4c7023126b783dd79e7ed47be31e1b76f975
Gerrit-Change-Number: 33652
Gerrit-PatchSet: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
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:57:37 +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
Attention is currently required from: pespin.
arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33202 )
Change subject: examples: Add extension header octet to example
......................................................................
Patch Set 13:
(1 comment)
File include/osmocom/netif/ipa.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33202/comment/1f9edb38_b85dcf99
PS12, Line 22: struct msgb *osmo_ipa_ext_msg_alloc(int headroom);
> this should imho be a un insigned int headroom or a size_t (yes I know the older API uses int, but i […]
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33202
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I962b9edcba65cdc98da00d2f8753dc5acd481502
Gerrit-Change-Number: 33202
Gerrit-PatchSet: 13
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 29 Jul 2023 18:57:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment