March 2024 - gerrit-log - lists.osmocom.org

[S] Change in libosmocore[master]: osmo_io: Avoid implementing non-existant situations

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/36193?usp=email ) Change subject: osmo_io: Avoid implementing non-existant situations ...................................................................... osmo_io: Avoid implementing non-existant situations Both of our back-ends have a register_fd and unregister_fd back-end. Let's simplify the code by not treating them as optional, which introduces code paths that we never take, adds small runtime overhead and makes the code harder to follow. Should we ever introduce more backends which might not need those call-backs, we can either have empty functions or think about how to make them optional. Change-Id: I0077151eb676f61320b3fa2124448852aa8fd4a9 --- M src/core/osmo_io.c 1 file changed, 23 insertions(+), 7 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/93/36193/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index 0b639fb..222f728 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -99,6 +99,8 @@ } OSMO_ASSERT(osmo_iofd_ops.close); + OSMO_ASSERT(osmo_iofd_ops.register_fd); + OSMO_ASSERT(osmo_iofd_ops.unregister_fd); OSMO_ASSERT(osmo_iofd_ops.write_enable); OSMO_ASSERT(osmo_iofd_ops.write_disable); OSMO_ASSERT(osmo_iofd_ops.read_enable); @@ -689,8 +691,7 @@ return -EBADF; } - if (osmo_iofd_ops.register_fd) - rc = osmo_iofd_ops.register_fd(iofd); + rc = osmo_iofd_ops.register_fd(iofd); if (rc) return rc; @@ -714,11 +715,7 @@ */ int osmo_iofd_unregister(struct osmo_io_fd *iofd) { - if (osmo_iofd_ops.unregister_fd) - return osmo_iofd_ops.unregister_fd(iofd); - IOFD_FLAG_SET(iofd, IOFD_FLAG_CLOSED); - - return 0; + return osmo_iofd_ops.unregister_fd(iofd); } /*! Get the number of messages in the tx queue. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36193?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: I0077151eb676f61320b3fa2124448852aa8fd4a9 Gerrit-Change-Number: 36193 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_io: avoid OSMO_ASSERT one each API call

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/36192?usp=email ) Change subject: osmo_io: avoid OSMO_ASSERT one each API call ...................................................................... osmo_io: avoid OSMO_ASSERT one each API call There's only one way to set the osmo_iofd_ops, which is by environment variable during the constructor time at shared library load time. There's hence no point in doing OSMO_ASSERT() on each and every call to osmo_iofd_notify_connected() at runtime. We can move those kind of asserts to the one-time load-time constructor instead. At the same time, we can extend those asserts to all the mandatory call-backs to be provided by the backend. Change-Id: Id9005ac6bb260236c88670373816bf7ee6a627f1 --- M src/core/osmo_io.c 1 file changed, 26 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/92/36192/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index e4807e5..0b639fb 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -98,6 +98,13 @@ exit(1); } + OSMO_ASSERT(osmo_iofd_ops.close); + OSMO_ASSERT(osmo_iofd_ops.write_enable); + OSMO_ASSERT(osmo_iofd_ops.write_disable); + OSMO_ASSERT(osmo_iofd_ops.read_enable); + OSMO_ASSERT(osmo_iofd_ops.read_disable); + OSMO_ASSERT(osmo_iofd_ops.notify_connected); + osmo_iofd_init(); } @@ -781,7 +788,6 @@ iofd->pending = NULL; - OSMO_ASSERT(osmo_iofd_ops.close); rc = osmo_iofd_ops.close(iofd); iofd->fd = -1; return rc; @@ -920,7 +926,6 @@ { OSMO_ASSERT(iofd->mode == OSMO_IO_FD_MODE_READ_WRITE || iofd->mode == OSMO_IO_FD_MODE_RECVMSG_SENDMSG); - OSMO_ASSERT(osmo_iofd_ops.notify_connected); osmo_iofd_ops.notify_connected(iofd); } -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36192?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: Id9005ac6bb260236c88670373816bf7ee6a627f1 Gerrit-Change-Number: 36192 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_io: Don't pretend to support backends without close_cb

by laforge

laforge has uploaded this change for review. ( 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(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/91/36191/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index 0e0a468..e4807e5 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -781,8 +781,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: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

[S] Change in libosmo-netif[master]: stream_cli: Correctly setup and free osmo_io client instance

by jolly

Attention is currently required from: daniel, dexter, laforge, pespin. jolly 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: > First, the iofd must be freed and cannot be reused, otherwise it will not function correctly. […] Done -- 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: laforge <laforge(a)osmocom.org> 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 09:30:34 +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

1 year, 5 months

1
0
0 0

[S] Change in libosmo-netif[master]: stream_cli: Correctly setup and free osmo_io client instance

by jolly

Attention is currently required from: daniel, dexter, laforge, pespin. jolly 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 7: (1 comment) Patchset: PS7: > I think the question daniel is racing:In the old code (pre 7e6d2e0f99ff095f4714f03b1ed991d6c9cb9c61) […] First, the iofd must be freed and cannot be reused, otherwise it will not function correctly. And yes, if osmo_stream_cli_open() is called again without closing, it runs into a memory leak. To guard against that, I added osmo_stream_cli_close_iofd() before calling osmo_iofd_setup(). -- 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: 7 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: laforge <laforge(a)osmocom.org> 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 09:30:18 +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

1 year, 5 months

1
0
0 0

[S] Change in libosmo-netif[master]: stream_{client,server} example: Cleanup on exit

by jolly

Attention is currently required from: jolly, pespin. Hello Jenkins Builder, daniel, laforge, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/36124?usp=email to look at the new patch set (#8). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder The change is no longer submittable: Verified is unsatisfied now. Change subject: stream_{client,server} example: Cleanup on exit ...................................................................... stream_{client,server} example: Cleanup on exit In order to detect memory leaks while debugging, stream server/client and keyboard is closed on exit. Related: OS#5753 Change-Id: I9dbb7f46b2a798e88ad4df8ff73c6ee40c07b843 --- M examples/stream-client.c M examples/stream-server.c 2 files changed, 34 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/24/36124/8 -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/36124?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: I9dbb7f46b2a798e88ad4df8ff73c6ee40c07b843 Gerrit-Change-Number: 36124 Gerrit-PatchSet: 8 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 year, 5 months

1
0
0 0

[S] Change in libosmo-netif[master]: stream_cli: Correctly setup and free osmo_io client instance

by jolly

Attention is currently required from: daniel, dexter, jolly, laforge, pespin. Hello Jenkins Builder, daniel, dexter, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-netif/+/35979?usp=email to look at the new patch set (#8). The following approvals got outdated and were removed: Code-Review+1 by dexter, Code-Review+1 by laforge, Code-Review+1 by pespin, Code-Review-1 by daniel, Verified+1 by Jenkins Builder Change subject: stream_cli: Correctly setup and free osmo_io client instance ...................................................................... stream_cli: Correctly setup and free osmo_io client instance Free osmo_io instance when calling osmo_stream_cli_close(). Also free osmo_io instance when calling osmo_stream_cli_open() if not freed, to prevent memory leaks. osmo_iofd_notify_connected() must be called before any registration of read or write, because osmo_io_iouring does not allow this. Change-Id: I91a6a76b9ff96034a7b333edf87af27490202932 --- M src/stream_cli.c 1 file changed, 22 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/79/35979/8 -- 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: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 year, 5 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_io: Log error message in case call-backs incompatible with mode

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/36190?usp=email ) Change subject: osmo_io: Log error message in case call-backs incompatible with mode ...................................................................... osmo_io: Log error message in case call-backs incompatible with mode Change-Id: I50ba6a76c0144f249d67488874a6c4edf01ec6f2 --- M src/core/osmo_io.c 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/90/36190/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index c1d206b..0631364 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -619,8 +619,11 @@ return NULL; } - if (ioops && !check_mode_callback_compat(mode, ioops)) + if (ioops && !check_mode_callback_compat(mode, ioops)) { + LOGP(DLIO, LOGL_ERROR, "iofd(%s): rejecting call-backs incompatible with mode\n", + name ? name : "unknown"); return NULL; + } iofd = talloc_zero(ctx, struct osmo_io_fd); if (!iofd) @@ -875,8 +878,10 @@ * \param[in] ioops osmo_io_ops structure to be set */ int osmo_iofd_set_ioops(struct osmo_io_fd *iofd, const struct osmo_io_ops *ioops) { - if (!check_mode_callback_compat(iofd->mode, ioops)) + if (!check_mode_callback_compat(iofd->mode, ioops)) { + LOGPIO(iofd, LOGL_ERROR, "rejecting call-backs incompatible with mode\n"); return -EINVAL; + } iofd->io_ops = *ioops; -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36190?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: I50ba6a76c0144f249d67488874a6c4edf01ec6f2 Gerrit-Change-Number: 36190 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_io: Guard osmo_iofd_register() with invalid file descriptor

by laforge

laforge has uploaded this change for review. ( 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, 19 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/88/36188/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index 5a5e05c..d5fd008 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -673,6 +673,12 @@ if (fd >= 0) iofd->fd = fd; + 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); if (rc) -- 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: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_iofd_register: Guard against calls from wrong state

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/36189?usp=email ) Change subject: osmo_iofd_register: Guard against calls from wrong state ...................................................................... osmo_iofd_register: Guard against calls from wrong state Right now the IOFD_FLAG_CLOSED flag represents a mixture of both closed and registered state. What we want to prevent against is repeat calls to osmo_iofd_register() of an io_fd that's already registered. Since osmo_iofd_register() itself will un-set the IOFD_FLAG_CLOSED, we can use it to guard against such invalid duplicate registration attempts. Change-Id: I422fc5eded41831663413e7118d4e012013a9ac9 --- M src/core/osmo_io.c 1 file changed, 24 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/89/36189/1 diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index d5fd008..c1d206b 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -670,6 +670,12 @@ { int rc = 0; + /* guard against repeat calls to osmo_iofd_register from the wrong state. */ + if (!IOFD_FLAG_ISSET(iofd, IOFD_FLAG_CLOSED)) { + LOGPIO(iofd, LOGL_ERROR, "Attempting to re-register an already registered io_fd\n"); + return -EINVAL; + } + if (fd >= 0) iofd->fd = fd; -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36189?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: I422fc5eded41831663413e7118d4e012013a9ac9 Gerrit-Change-Number: 36189 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 5 months

1
0
0 0

2025

2024

2023

2022

gerrit-log March 2024