Attention is currently required from: arehbein, laforge, pespin.
daniel has uploaded a new patch set (#9) to the change originally created by arehbein. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Add osmo_io support to osmo_stream_cli and osmo_stream_srv
Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
---
M include/osmocom/netif/stream.h
M src/stream.c
2 files changed, 304 insertions(+), 28 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/93/33193/9
--
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: 9
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-MessageType: newpatchset
Attention is currently required from: arehbein, laforge, 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 7:
(8 comments)
Patchset:
PS5:
> > > Since we already have osmo_stream_*_get_ofd() we should add osmo_stream_*_get_iofd(). […]
I see no problem with having separate APIs regarding what you mention. The socket may be already enabled, but it may not trigger any callback until you return to the mainloop, so you can perfectly call the APIs to set the callbacks before they are called.
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/a95e90f8_70ea30f0
PS7, Line 554: static void handle_connecting(struct osmo_stream_cli *cli, int res)
this is for osmo_stream_cli, so adding a "stream_cli" prefix would help easily gastp that.
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/a446295c_14b4e708
PS7, Line 598: handle_connecting(cli, res);
This split into a separate function you could do it in a previous separate preparation patch.
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/078ba248_8761d154
PS7, Line 620: if (msg && res <= 0) {
EAGAIN would trigger closing the socket?
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/591fa510_4e5313db
PS7, Line 881: cli->ofd.priv_nr = 0; /* XXX */
what about this XXX?
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/95a2a986_31e7cdd9
PS7, Line 1046: cli->iofd = osmo_iofd_setup(cli, -1, cli->name, OSMO_IO_FD_MODE_READ_WRITE, &osmo_stream_cli_ioops, cli);
shouldn't this -1 be "fd"?
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/242de708_26f4a629
PS7, Line 1540: osmo_stream_srv_destroy(conn);
just wondering whether you may need to return -EBADF here?
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/fa73f28d_48f27e8e
PS7, Line 1714: conn->iofd_read_cb = read_cb;
See, these can perfectly be separate setter APIs like we do for osmo_stream_cli.
--
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: 7
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: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 09 Jun 2023 12:41:28 +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: arehbein, pespin, daniel.
Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33194 )
Change subject: examples: Use new stream API in {ipa-,}stream-{client,server}
......................................................................
Patch Set 6:
(2 comments)
File examples/stream-client.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-8120):
https://gerrit.osmocom.org/c/libosmo-netif/+/33194/comment/a5c617e1_aff611b1
PS6, Line 53: static int read_cb(struct osmo_stream_cli *conn, struct msgb* msg)
"foo* bar" should be "foo *bar"
File examples/stream-server.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-8120):
https://gerrit.osmocom.org/c/libosmo-netif/+/33194/comment/a5ff3e2b_8c7e5880
PS6, Line 45: int read_cb(struct osmo_stream_srv *conn, struct msgb* msg)
"foo* bar" should be "foo *bar"
--
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: 6
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: Fri, 09 Jun 2023 12:29:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin, daniel.
daniel has uploaded a new patch set (#6) to the change originally created by arehbein. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33194 )
Change subject: examples: Use new stream API in {ipa-,}stream-{client,server}
......................................................................
examples: Use new stream API in {ipa-,}stream-{client,server}
Change-Id: I97a9979199c816686b32080534627f6f033e009e
---
M examples/ipa-stream-client.c
M examples/ipa-stream-server.c
M examples/stream-client.c
M examples/stream-server.c
4 files changed, 28 insertions(+), 76 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/94/33194/6
--
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: 6
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-MessageType: newpatchset
Attention is currently required from: arehbein, laforge, pespin.
daniel has uploaded a new patch set (#8) to the change originally created by arehbein. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Add osmo_io support to osmo_stream_cli and osmo_stream_srv
Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
---
M include/osmocom/netif/stream.h
M src/stream.c
2 files changed, 344 insertions(+), 30 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/93/33193/8
--
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: 8
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-MessageType: newpatchset
Attention is currently required from: arehbein, laforge, pespin.
daniel has uploaded a new patch set (#7) to the change originally created by arehbein. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33193 )
Change subject: Add osmo_io support to osmo_stream_cli and osmo_stream_srv
......................................................................
Add osmo_io support to osmo_stream_cli and osmo_stream_srv
Change-Id: I2f52c7107c392b6f4b0bf2a84f8c873c084a200c
---
M include/osmocom/netif/stream.h
M src/stream.c
2 files changed, 342 insertions(+), 30 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/93/33193/7
--
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: 7
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-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 7:
(1 comment)
Patchset:
PS5:
> > Since we already have osmo_stream_*_get_ofd() we should add osmo_stream_*_get_iofd(). Since they will return pointer we could simply return NULL if it's called with the wrong mode, but maybe an assert is better.
>
> The question here would be: do we really need that API for iofd? or iofd can be handled totally internally?
Ok, it seems this is mostly used in the actual read_cb to get the fd to perform the read. Other uses also use the file descriptor with osmo_sock_get_name2().
So I guess we don't need it.
> > osmo_stream_srv_create2 is called with the newly accept()ed fd so I'd still pass the read/close callbacks in create2().
>
> I'd keep setting the callbacks as separate APIs. Yes, it does 2 function calls more, occupies 2 more lines, but if we had done this already in the past then now we wouldn't need to be changing this osmo_stream_srv_create() API in first place... so learning for errors = adding separate setter APIs.
Well, it's 3 more - setting the callbacks and then calling a function that actually enables reading from the fd.
You usually call osmo_stream_srv_create() inside of the connect_cb of osmo_strem_srv_link. At that point the fd is already connected since it has been accept()ed and might also have pending data.
Every osmo_stream_* where the fd is initially "inactive" has separate setter functions for the callbacks so this choice feels deliberate to me (and I agree with it).
--
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: 7
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: Fri, 09 Jun 2023 12:14:24 +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, dexter.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/32932
to look at the new patch set (#15).
Change subject: ASCI: Add message definition and encoding according to 3GPP TS 48.008
......................................................................
ASCI: Add message definition and encoding according to 3GPP TS 48.008
Change-Id: Ib94c64136c31ce4af67c314a8550d93946cc844f
---
M include/osmocom/gsm/gsm0808.h
M include/osmocom/gsm/protocol/gsm_08_08.h
M src/gsm/gsm0808.c
M src/gsm/libosmogsm.map
4 files changed, 933 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/32932/15
--
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: 15
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: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: jolly, dexter.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/32932
to look at the new patch set (#14).
Change subject: ASCI: Add message definition and encoding according to 3GPP TS 48.008
......................................................................
ASCI: Add message definition and encoding according to 3GPP TS 48.008
Change-Id: Ib94c64136c31ce4af67c314a8550d93946cc844f
---
M include/osmocom/gsm/gsm0808.h
M include/osmocom/gsm/protocol/gsm_08_08.h
M src/gsm/gsm0808.c
M src/gsm/libosmogsm.map
4 files changed, 931 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/32932/14
--
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: 14
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: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: laforge.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33235 )
Change subject: test_ota: Add one first OTA SMS AES128 unit test
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33235
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: Id4a66bbfaec2d8610e8a7a2c72c0dfd08332edcd
Gerrit-Change-Number: 33235
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Fri, 09 Jun 2023 11:00:48 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment