Attention is currently required from: daniel.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-netif/+/33221
to look at the new patch set (#6).
Change subject: stream: Update log messages
......................................................................
stream: Update log messages
Change-Id: Ife20b9d18e6ca86a06991d68165694e31052c58a
---
M src/stream.c
M tests/stream/stream_test.err
2 files changed, 17 insertions(+), 10 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/21/33221/6
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33221
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ife20b9d18e6ca86a06991d68165694e31052c58a
Gerrit-Change-Number: 33221
Gerrit-PatchSet: 6
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: arehbein, laforge, pespin.
daniel has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Patch Set 6:
(2 comments)
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/1bbd6fe2_ef095c7b
PS1, Line 1758: OSMO_ASSERT
> I don't know the code like you do, but I generally agree with your point here. […]
Done
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/fcde1c9d_0e7f0934
PS5, Line 1197: static int osmo_stream_srv_ofd_cb(struct osmo_fd *ofd, unsigned int what)
> I'd welcome if you did this kind of changes in separate previous patches to try to trim this one a b […]
Done
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33193
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
Gerrit-Change-Number: 33193
Gerrit-PatchSet: 6
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 15:10:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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: jolly, pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/32932 )
Change subject: ASCI: Add message definition and encoding according to 3GPP TS 48.008
......................................................................
Patch Set 12:
(3 comments)
File include/osmocom/gsm/protocol/gsm_08_08.h:
https://gerrit.osmocom.org/c/libosmocore/+/32932/comment/39e5ed28_426525f1
PS12, Line 175: BSS_MAP_MSG_UPLINK_REJECT_CMD = 75,
(not critical: you only change the formatting here. Usually we put those changes in a follow up patch)
File src/gsm/gsm0808.c:
https://gerrit.osmocom.org/c/libosmocore/+/32932/comment/b7180209_33b546bc
PS12, Line 22: #ifdef HAVE_SYS_SOCKET_H
As far as I understand from the discussion around patch-set 5, this is to prevent building this code on embedded targets. I would split this part into a patch preceding this one.
File tests/tlv/tlv_test.c:
https://gerrit.osmocom.org/c/libosmocore/+/32932/comment/2ffe244f_1a500d76
PS12, Line 4:
TLV parsing should work on embedded targets. I can see that this has ties to gsm0808 (osmo_bssap_tlv_parse). If we could remove this dependency then we could include the tlv_test in embedded builds again.
I also see an additional problem: This is a unit test. We probably run into problems when we just remove this for some builds. The output will change and no longer match the .ok and .err files. (But the build passes in jenkins, maybe it isn't a problem though...)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/32932
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ib94c64136c31ce4af67c314a8550d93946cc844f
Gerrit-Change-Number: 32932
Gerrit-PatchSet: 12
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: dexter <pmaier(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 15:06:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin, daniel.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33194 )
Change subject: examples: Use osmo_io in {ipa-,}stream-{client,server}
......................................................................
Patch Set 4:
(1 comment)
File examples/ipa-stream-client.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33194/comment/f5ca0579_63a725b4
PS4, Line 176: osmo_stream_cli_set_iofd_read_cb(conn, read_cb);
> I don't really like polluting the APIs in osmo_stream with implementation specific "iofd" names. […]
I tend to agree with pau.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33194
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I97a9979199c816686b32080534627f6f033e009e
Gerrit-Change-Number: 33194
Gerrit-PatchSet: 4
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: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 15:01:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin, daniel.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
> implementation of osmo_stream should always use iofd, hence the old implementation is dropped (or #if 0 or ENV disabled for now).
I'd love to see that, given that osmo_io has the select/poll backend anyway for backwards compatibility.
The problem I see is: How would you handle compatibility with old code that registers a read_cb? That old code expects to do the actual read itself, from the file descriptor. But in osmo_io that already has happened by the infrastructure at the tiem you get to the read_cb...
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33193
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
Gerrit-Change-Number: 33193
Gerrit-PatchSet: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 15:01:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein, daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33194 )
Change subject: examples: Use osmo_io in {ipa-,}stream-{client,server}
......................................................................
Patch Set 4:
(1 comment)
File examples/ipa-stream-client.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33194/comment/3cf4fed5_e161ca7f
PS4, Line 176: osmo_stream_cli_set_iofd_read_cb(conn, read_cb);
I don't really like polluting the APIs in osmo_stream with implementation specific "iofd" names. Rather call them (since they are the preferred way from now on to use the objects):
osmo_stream_cli_create2
osmo_stream_cli_set_read_cb2 (or osmo_stream_cli_set_read_cb_msg)
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33194
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I97a9979199c816686b32080534627f6f033e009e
Gerrit-Change-Number: 33194
Gerrit-PatchSet: 4
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: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 14:33:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33221 )
Change subject: stream: Update log messages
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
Patchset:
PS5:
Better move this small fixup commits before the main commit so that we already have them merged and the "complex iofd patch" looks easier to gasp.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33221
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: Ife20b9d18e6ca86a06991d68165694e31052c58a
Gerrit-Change-Number: 33221
Gerrit-PatchSet: 5
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 14:31:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: arehbein, daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS5:
> Proposal: […]
If you want to still keep the old ofd behavior, you could even set the ofd/iofd type internally based on whether the new osmo_stream_*_set_read_cb() was called.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33193
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
Gerrit-Change-Number: 33193
Gerrit-PatchSet: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 14:29:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein, daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Patch Set 5:
(2 comments)
Patchset:
PS5:
Proposal:
- implementation of osmo_stream should always use iofd, hence the old implementation is dropped (or #if 0 or ENV disabled for now).
- the old _create() functions with read_cb and close_cb are kept for backward compat
- a new _create() function is added, which has no read_cb nor close_cb, but a const char *name.
- Separate APIs are added to set the new iofd_read_cb and iofd_close_cb.
- Internally, the osmo_stream code calls the read_cb() if non NULL, or/and the iofd_read_cb() if not NULL (the only difference afaiu is the extra msg ptr being passed).
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/4bf8852b_01fcb000
PS5, Line 1197: static int osmo_stream_srv_ofd_cb(struct osmo_fd *ofd, unsigned int what)
I'd welcome if you did this kind of changes in separate previous patches to try to trim this one a bit so we can focus on how to add iofd in here.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33193
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
Gerrit-Change-Number: 33193
Gerrit-PatchSet: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 08 Jun 2023 14:28:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment