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 10:
(6 comments)
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/5368ac86_5373c2e5
PS7, Line 598: handle_connecting(cli, res);
> This split into a separate function you could do it in a previous separate preparation patch.
Ack
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/6c84d18b_b1b934e3
PS7, Line 620: if (msg && res <= 0) {
> EAGAIN would trigger closing the socket?
This shouldn't happen since osmo_io only writes to the fd if it's writable.
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/bc4afef1_4581ada5
PS7, Line 881: cli->ofd.priv_nr = 0; /* XXX */
> what about this XXX?
Done
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/b6c5350a_6c95c2c9
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"?
The relevant fd is in osmo_iofd_register(). In the reconnect case when osmo_stream_cli_open is called cli->iofd will already be set and osmo_iofd_register() registers a new fd.
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/7a08bcd5_9d49e29b
PS7, Line 1540: osmo_stream_srv_destroy(conn);
> just wondering whether you may need to return -EBADF here?
Not in a void function :-)
osmo_stream_srv_destroy() calls osmo_iofd_free() which does the right thing™
It'll close the fd, defer free() until after the callback has returned and not call the other callback.
https://gerrit.osmocom.org/c/libosmo-netif/+/33193/comment/b6e69d6f_6b476914
PS7, Line 1714: conn->iofd_read_cb = read_cb;
> See, these can perfectly be separate setter APIs like we do for osmo_stream_cli.
I never argued that they couldn't be
--
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: 10
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 14:07:20 +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: jolly.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33144 )
Change subject: BSSMAP_Templates: Add templates to receive CellID IE
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
File library/BSSMAP_Templates.ttcn:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33144/comment/b86717f5_fca3…
PS3, Line 371: present
> There's still something wrong with this template. […]
We had a call with @jolly and agreed that it's better to accept MCC/MNC pair in form of already encoded blob. This obligates the caller to call `f_enc_mcc_mnc()` when using this template, but on the other hand allows passing `?`. Marking as resolved.
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33144/comment/ca30e74c_d6ad…
PS3, Line 1467: GsmMcc mcc, GsmMnc mnc
> Here I suggest keeping both `mcc` and `mnc` as they are, i.e. value parameters. […]
Done
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33144
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I42d52d871c8011db7e0897dfe752afeefa6d9662
Gerrit-Change-Number: 33144
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Fri, 09 Jun 2023 14:04:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: jolly.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33256 )
Change subject: Added generation of include/osmocom/core/socket_compat.h
......................................................................
Patch Set 2:
(1 comment)
File include/osmocom/core/Makefile.am:
https://gerrit.osmocom.org/c/libosmocore/+/33256/comment/f48746f8_fab97aa5
PS2, Line 102: EXTRA_DIST = socket_compat.h.tpl
we usually put this above, around line 83
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33256
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ia3eafc992221900bbbf1760f669725bf9da92105
Gerrit-Change-Number: 33256
Gerrit-PatchSet: 2
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Fri, 09 Jun 2023 14:03:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: jolly.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/33256
to look at the new patch set (#2).
Change subject: Added generation of include/osmocom/core/socket_compat.h
......................................................................
Added generation of include/osmocom/core/socket_compat.h
This file is required to compile header files on machines that do not
have sys/socket.h.
Change-Id: Ia3eafc992221900bbbf1760f669725bf9da92105
---
M configure.ac
M include/osmocom/core/Makefile.am
A include/osmocom/core/socket_compat.h.tpl
3 files changed, 30 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/56/33256/2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33256
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ia3eafc992221900bbbf1760f669725bf9da92105
Gerrit-Change-Number: 33256
Gerrit-PatchSet: 2
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-MessageType: newpatchset
Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33254 )
Change subject: stream: Use cli->state to check if cli is already closed
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33254
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I92231528da08f8891e20c1226b61989a65e00ccd
Gerrit-Change-Number: 33254
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 09 Jun 2023 13:55:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: daniel.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33253 )
Change subject: stream: Factor out reconnection handling
......................................................................
Patch Set 1:
(1 comment)
File src/stream.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/33253/comment/493917bb_df80959a
PS1, Line 472: osmo_fd_write_disable(&cli->ofd);
what if connect_cb() freed the cli object? you'd be accessing it here. This needs to be moved inside stream_cli_handle_connecting at the proper place.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33253
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I418377eabd465ee4ffce9b4440e96287c7734924
Gerrit-Change-Number: 33253
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 09 Jun 2023 13:55:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: jolly.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33256 )
Change subject: Added generation of include/osmocom/core/socket_compat.h
......................................................................
Patch Set 1:
(1 comment)
File include/osmocom/core/Makefile.am:
https://gerrit.osmocom.org/c/libosmocore/+/33256/comment/87330990_25616806
PS1, Line 98: socket_compat.h.tpl
this likely needs to go into EXTRA_DIST or something along those lines. Otherwise "make dist" will not add it to the tarball. "make distcheck" should tell.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33256
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ia3eafc992221900bbbf1760f669725bf9da92105
Gerrit-Change-Number: 33256
Gerrit-PatchSet: 1
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-CC: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Fri, 09 Jun 2023 13:49:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment