Attention is currently required from: arehbein.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33201 )
Change subject: stream: Add IPA send function, pull IPA headers in segm. cb
......................................................................
Patch Set 10:
(2 comments)
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/e2ab6663_41fadecf
PS10, Line 475: void ipa_stream_srv_send
looks like this is this a new public function without osmo_ prefix?
https://gerrit.osmocom.org/c/libosmo-netif/+/33201/comment/7d448e93_790721f7
PS10, Line 494: void ipa_stream_cli_send(struct osmo_stream_cli *cli, int ipaccess_proto,
looks like this is this a new public function without osmo_ prefix?
--
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: 10
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 09 Jul 2023 07:16:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin.
laforge 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 9:
(2 comments)
File include/osmocom/netif/stream.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/7ad1feea_f823f0ee
PS9, Line 63: #define osmo_stream_srv_unset_segmentation_cb(STRUCT_OSMO_STREAM_SRV_PTR)\
: osmo_stream_srv_set_segmentation_cb((STRUCT_OSMO_STREAM_SRV_PTR), NULL)
If we want this, I would put it in a function and export it. This is not a performance critical path where we need to in-line or work with #defines.
Having a function call in the source code be transparently changed into another function call can make debugging, tracing, profiling, ... harder - with no apparent benefit (in this case, at least).
File include/osmocom/netif/stream.h:
https://gerrit.osmocom.org/c/libosmo-netif/+/33197/comment/c001c40f_4d4c7b3f
PS1, Line 25: osmo_stream_proto
> I have for now put `osmo_stream_trx_mode` instead; the name should be more general than just referri […]
TRX unfortunately has a very specific meaning in the context of GSM, so it is at least as bad as "protocol" in terms of a socket API :( We'll have to find something better.
--
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: 9
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: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 09 Jul 2023 07:12:32 +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>
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33652 )
Change subject: ipa: Add segmentation callback
......................................................................
Patch Set 2:
(2 comments)
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33652/comment/c2295e0a_11bb3927
PS2, Line 378: MSG_CB_IPA_PROTO_EXT_IS_SET_OFFSET
can you explain those variables more? "MSGB_CB_IPA_PROTO_EXT is clear but what is "IS_SET_OFFSET"? Sounds like a predicate meaning "the offset is of type 'set'" but that doesn't make sense..
https://gerrit.osmocom.org/c/libosmo-netif/+/33652/comment/ceff8f23_ac2c1c68
PS2, Line 432: /* Below: Helper functions for addition of send_ipa functionality in later commit */
it's weird to introduce static inline helper functions in a commit thta doesn't use those.
--
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: 2
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 09 Jul 2023 07:09:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33206 )
Change subject: stream (cosmetic): Fix osmo_panic log fmts
......................................................................
Patch Set 8: Code-Review+1
(3 comments)
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33206/comment/82787bb9_6a043b1f
PS6, Line 615: cli->state);
> there's also the __func__ or __FUNCTION__ macro that could be used here. […]
Done
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33206/comment/1a6dd28c_88af7da5
PS8, Line 613: osmo_panic("osmo_stream_cli_write_cb() called with unexpected state %d\n", cli->state);
why the line wrap here ? having this in one line certainly doesn't exceed the permitted line length.
https://gerrit.osmocom.org/c/libosmo-netif/+/33206/comment/3ed57ea9_670768c8
PS8, Line 633: }
why the line wrap here ? having this in one line certainly doesn't exceed the permitted line length.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33206
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Id082a9473b788f8de20cdc2ba4430b3289f4ce5a
Gerrit-Change-Number: 33206
Gerrit-PatchSet: 8
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: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 09 Jul 2023 07:06:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: arehbein.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33655 )
Change subject: msgb: Add helper macro to decide if a segment is incomplete
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
I think this *highly* specific to one use case and better be defined in the context of this use case.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33655
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ifc7cf0629e93864bfb3e36c64143d4ccc777e4b6
Gerrit-Change-Number: 33655
Gerrit-PatchSet: 1
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Comment-Date: Sun, 09 Jul 2023 07:04:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment