Attention is currently required from: laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email )
Change subject: [cosmetic] re-order hnbgw.c to group code in major blocks
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Sorry, not now, I have just spent multiple hours rebasing/weaving/merge-conflict-resolving. […]
Fine, just sharing thoughts.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ia7ce60e6f80d10b7712de1aa6d8a30dd61690dcc
Gerrit-Change-Number: 36080
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 07 Mar 2024 17:35:44 +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>
Gerrit-MessageType: comment
Attention is currently required from: daniel, dexter, jolly.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email )
Change subject: stream_cli: Correctly setup and free osmo_io client instance
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
File src/stream_cli.c:
https://gerrit.osmocom.org/c/libosmo-netif/+/35979/comment/3b620ec1_8f176641
PS8, Line 950: #ifdef HAVE_LIBSCTP
All this code starting from here and below can be cleaned up a bit in another patch. It's difficult to follow with the ifdefs and the "if (!cli->iofd)" afterwards...
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I91a6a76b9ff96034a7b333edf87af27490202932
Gerrit-Change-Number: 35979
Gerrit-PatchSet: 8
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: dexter <pmaier(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-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 07 Mar 2024 17:32:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: pespin.
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email
to look at the new patch set (#2).
The following approvals got outdated and were removed:
Verified-1 by Jenkins Builder
Change subject: [cosmetic] re-order hnbgw.c to group code in major blocks
......................................................................
[cosmetic] re-order hnbgw.c to group code in major blocks
Change-Id: Ia7ce60e6f80d10b7712de1aa6d8a30dd61690dcc
---
M src/osmo-hnbgw/hnbgw.c
1 file changed, 203 insertions(+), 173 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/80/36080/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ia7ce60e6f80d10b7712de1aa6d8a30dd61690dcc
Gerrit-Change-Number: 36080
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email )
Change subject: [cosmetic] re-order hnbgw.c to group code in major blocks
......................................................................
Patch Set 1:
(1 comment)
This change is ready for review.
Patchset:
PS1:
> You could even take the chance to split hnb_context to its own file. […]
Sorry, not now, I have just spent multiple hours rebasing/weaving/merge-conflict-resolving. That should be done at a time when there's not significant changes piled on top of it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36080?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ia7ce60e6f80d10b7712de1aa6d8a30dd61690dcc
Gerrit-Change-Number: 36080
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 07 Mar 2024 17:25:16 +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: daniel, dexter, jolly, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email )
Change subject: stream_cli: Correctly setup and free osmo_io client instance
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
Patchset:
PS7:
> I don't get it. osmo_iofd_setup is somtehing you call once for allocation and configuration. […]
ok, so normally osmo_stream_cli_create() should be calling osmo_iofd_setup(). In osmo_stream_cli_open() we should only call osmo_iofd_register(). That way we could do any number of open/close (vs. register/unregister) cycles without having to allocate a new iofd.
The reason we're not using osmo_iofd as it is expected to use is probably because in osmo_stream_cli_create() we don't yet know the mode, and hence don't know if we should go the osmo_fd or the osmo_iofd route.
Therefore we need to work-around this by doing osmo_iofd_setup() every time we go through osmo_stream_cli_open() - and hence we also need to free it. Sad, but we need to work around the mode not being known earlier. We might add some comments to explain the unusual use of osmo_iofd
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I91a6a76b9ff96034a7b333edf87af27490202932
Gerrit-Change-Number: 35979
Gerrit-PatchSet: 8
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: dexter <pmaier(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-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 07 Mar 2024 16:45:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: daniel, dexter, jolly, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email )
Change subject: stream_cli: Correctly setup and free osmo_io client instance
......................................................................
Patch Set 8:
(1 comment)
Patchset:
PS7:
> osmo_iofd_setup() creates a new osmi_io instance. […]
I don't get it. osmo_iofd_setup is somtehing you call once for allocation and configuration. you can then many times do osmo_iofd_register/unregister/register/unregister with different file-descriptors. Why do we need more functions as you say?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I91a6a76b9ff96034a7b333edf87af27490202932
Gerrit-Change-Number: 35979
Gerrit-PatchSet: 8
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: dexter <pmaier(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-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 07 Mar 2024 16:40:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: comment
laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36188?usp=email )
Change subject: osmo_io: Guard osmo_iofd_register() with invalid file descriptor
......................................................................
osmo_io: Guard osmo_iofd_register() with invalid file descriptor
Let's return an error if both osmo_iofd_setup() and osmo_iofd_register()
are called with an invalid file descriptor like -1. Either one of them
must have been called with a valid file descriptor.
Change-Id: Ie4561cefad82e1bf5d37dd1a4815f4bc805343e6
---
M src/core/osmo_io.c
1 file changed, 18 insertions(+), 0 deletions(-)
Approvals:
dexter: Looks good to me, but someone else must approve
Jenkins Builder: Verified
pespin: Looks good to me, but someone else must approve
jolly: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c
index 5a5e05c..f0d213c 100644
--- a/src/core/osmo_io.c
+++ b/src/core/osmo_io.c
@@ -672,6 +672,11 @@
if (fd >= 0)
iofd->fd = fd;
+ else if (iofd->fd < 0) {
+ /* this might happen if both osmo_iofd_setup() and osmo_iofd_register() are called with -1 */
+ LOGPIO(iofd, LOGL_ERROR, "Cannot register io_fd using invalid fd == %d\n", iofd->fd);
+ return -EBADF;
+ }
if (osmo_iofd_ops.register_fd)
rc = osmo_iofd_ops.register_fd(iofd);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36188?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ie4561cefad82e1bf5d37dd1a4815f4bc805343e6
Gerrit-Change-Number: 36188
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36191?usp=email )
Change subject: osmo_io: Don't pretend to support backends without close_cb
......................................................................
osmo_io: Don't pretend to support backends without close_cb
Let's not pretend we support backends without a close_cb. In such
situations nobody would actually close(2) the file descriptor,
but we would set iofd->fd to -1, effectively creating a file descriptor
leak.
Both of our two back-ends provide a close_cb, and we don't need to
consider hypothetical future back-ends that would not like to register
such a call-back.
Related: OS#6393
Change-Id: Id285f1d7b73ae5805aa618897016ae8b73bf892d
---
M src/core/osmo_io.c
1 file changed, 21 insertions(+), 2 deletions(-)
Approvals:
Jenkins Builder: Verified
pespin: Looks good to me, but someone else must approve
jolly: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c
index bcd4add..8b53aa6 100644
--- a/src/core/osmo_io.c
+++ b/src/core/osmo_io.c
@@ -787,8 +787,8 @@
iofd->pending = NULL;
- if (osmo_iofd_ops.close)
- rc = osmo_iofd_ops.close(iofd);
+ OSMO_ASSERT(osmo_iofd_ops.close);
+ rc = osmo_iofd_ops.close(iofd);
iofd->fd = -1;
return rc;
}
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36191?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id285f1d7b73ae5805aa618897016ae8b73bf892d
Gerrit-Change-Number: 36191
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged