Attention is currently required from: daniel.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33344 )
Change subject: stream: Set state to closed before calling disconnect_cb()
......................................................................
Patch Set 2: Verified+1 Code-Review+2
(1 comment)
Patchset:
PS1:
> I ran the ttcn3 cbc tests here but still hat +8 test failures. No coredump though. […]
I gave this patch a try, and AFAICS it fixes the problem. In my case only `TC_cell_failure_restart_active_bsc` and `TC_cell_failure_restart_active_mme` failed, which were known to fail before the regression.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33344
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I40ceb17c32d1f58f8d0eeda8d1d794cf3f478e83
Gerrit-Change-Number: 33344
Gerrit-PatchSet: 2
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 17 Jun 2023 11:02:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: arehbein, pespin, daniel.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33083 )
Change subject: gsm/ipa: Add segmentation callback
......................................................................
Patch Set 9:
(3 comments)
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/6e1c17af_700eabb2
PS2, Line 730: msg->data_len < total_len
> Seems better to me, as that would prevent more error cases
Ah, I see. I forgot that `msg->data_len` is the maximum length the buffer can hold, while `msg->len` is how much is used out of `msg->data_len`. Fine then.
File src/gsm/ipa.c:
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/dcc21e55_f1eb6463
PS8, Line 735: const struct ipaccess_head *hh = (const struct ipaccess_head *) msg->data;
> You could declare this at the start of the function (we usually declare variables at the start of th […]
Agreeing with Pau here. There is no strict requirement to declare variables at the top of the respective block, but in this specific case we can improve code readability a bit by doing `msgb_length(msg) < sizeof(*hh)` above.
https://gerrit.osmocom.org/c/libosmocore/+/33083/comment/11bc0665_6da6a3c8
PS8, Line 738: if (msgb_tailroom(msg) + msgb_length(msg) < total_len) {
> iiuc the problem here is that the allocated msgb space is not going to be enough to fit in what IPA message expects.
ack.
> Talking about that in the log message would be a lot more usefu for the user? since that probably means the dev has to increase the msgb size when allocating it?
Indeed, the logging message should be more specific. Something like `msgb (len=%zu) is not large enough to fit received IPA message (len=%zu)` maybe?
Also, this is not expected to happen too often, so `OSMO_UNLIKELY`?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33083
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I3a639e6896cc3b3fc8e9b2e1a58254710efa0d3f
Gerrit-Change-Number: 33083
Gerrit-PatchSet: 9
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: daniel <dwillmann(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(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: Sat, 17 Jun 2023 09:48:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
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: fixeria, daniel.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-netif/+/33344 )
Change subject: stream: Set state to closed before calling disconnect_cb()
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33344
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I40ceb17c32d1f58f8d0eeda8d1d794cf3f478e83
Gerrit-Change-Number: 33344
Gerrit-PatchSet: 2
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 17 Jun 2023 06:29:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: pespin, dexter.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/33312 )
Change subject: mgw: Allow auditing speciall 'null' endpoint
......................................................................
Patch Set 3:
(1 comment)
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/33312/comment/8c29417a_2f57b272
PS2, Line 473:
> The main discussion to start with would be: Do we still want the null endpoint to be per-trunk? That […]
I would think of the null endpoint as something global, not per-trunk
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/33312
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ia409b16e9211e6261e2e0f21288544289d6f3733
Gerrit-Change-Number: 33312
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 17 Jun 2023 06:28:40 +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: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria, daniel.
Hello Jenkins Builder, laforge, fixeria,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmo-netif/+/33344
to look at the new patch set (#2).
Change subject: stream: Set state to closed before calling disconnect_cb()
......................................................................
stream: Set state to closed before calling disconnect_cb()
Fixes recent crashes in ttcn-cbc-test
Change-Id: I40ceb17c32d1f58f8d0eeda8d1d794cf3f478e83
Related: OS#6063
---
M src/stream.c
1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/44/33344/2
--
To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/33344
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-Change-Id: I40ceb17c32d1f58f8d0eeda8d1d794cf3f478e83
Gerrit-Change-Number: 33344
Gerrit-PatchSet: 2
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin, daniel.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33341 )
Change subject: osmo_io: Make name optional, add _set_name() API
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> Adding Harald because in our early discussions we agreed to have the name mandatory in osmo_io. […]
I think the ability to change it is definitely good. Not sure if we should make it optional upon creation. Not sure what we gain by that, other than more objects in our code we don't have proper context for when logging or introspecting.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33341
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: If2772a3ccaa98616e0189862a49ab0243435e343
Gerrit-Change-Number: 33341
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Sat, 17 Jun 2023 06:12:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: daniel <dwillmann(a)sysmocom.de>
Gerrit-MessageType: comment