Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/32358 )
Change subject: ms: Make ms_{attach,detach}_tbf expectancies more robust
......................................................................
Patch Set 2:
(3 comments)
File src/gprs_ms.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/7a4cc895_fc963d33
PS2, Line 355: OSMO_ASSERT(ms);
> Somehow it feels wrong when I am seeing a function assert()ing all its pointer arguments. […]
it's not really the same. These are internal pointers, not stuff received from the peers over the wire. They are checking internal state, and help understand readers (and remind myself) of the expected assumptions.
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/24d56194_50a8847a
PS2, Line 409: OSMO_ASSERT(!ms_tbf_is_attached(ms, tbf));
> ... especially when some calling functions also do assert the arguments too.
Again, this is internal state. If it breaks it means something is fundamentally wrong in the model, or somebody made a change in the wrong direction. It also helps understand the current model.
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/4436d8ad_4fe7c7e4
PS2, Line 419: OSMO_ASSERT(tbf_ms(tbf) == ms);
> ... and here you're dereferencing tbf before assert()ing it.
I can add an OSMO_ASSERT(tbf) there if you want, but it's clear here that it is expected to be not NULL.
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/32358
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ia18fe2de1fb3bf72f530157e5f30de64f2b11e12
Gerrit-Change-Number: 32358
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Apr 2023 18:39:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-pcu/+/32397
to look at the new patch set (#2).
Change subject: alloc_algo: Pass a struct containing all req params
......................................................................
alloc_algo: Pass a struct containing all req params
This is a first step towards isolating the allocation algorithm from
applying changes on PCU state.
In next steps the tbf pointer will be dropped and the allocation
algorithm will only result a "result" struct which then the caller can
apply to whatever TBF object it requires.
Change-Id: Ie4d9ace526ad012d97738bc55bdb5cc1472c632d
---
M src/alloc_algo.cpp
M src/alloc_algo.h
M src/gprs_pcu.h
M src/pcu_vty_functions.cpp
M src/tbf.cpp
M src/tbf_dl.cpp
M tests/emu/pcu_emu.cpp
M tests/ulc/PdchUlcTest.cpp
8 files changed, 165 insertions(+), 132 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/97/32397/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/32397
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ie4d9ace526ad012d97738bc55bdb5cc1472c632d
Gerrit-Change-Number: 32397
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/32358 )
Change subject: ms: Make ms_{attach,detach}_tbf expectancies more robust
......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/ddcb1201_fe327e75
PS2, Line 9: attach
attached?
File src/gprs_ms.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/c4cf4d99_8aa069c9
PS2, Line 355: OSMO_ASSERT(ms);
Somehow it feels wrong when I am seeing a function assert()ing all its pointer arguments.
As if I am reading the source code of open5gs-pcud...
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/d9d2e04b_36450829
PS2, Line 409: OSMO_ASSERT(!ms_tbf_is_attached(ms, tbf));
... especially when some calling functions also do assert the arguments too.
https://gerrit.osmocom.org/c/osmo-pcu/+/32358/comment/14b78855_4be1082c
PS2, Line 419: OSMO_ASSERT(tbf_ms(tbf) == ms);
... and here you're dereferencing tbf before assert()ing it.
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/32358
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Ia18fe2de1fb3bf72f530157e5f30de64f2b11e12
Gerrit-Change-Number: 32358
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Apr 2023 18:13:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/32346 )
Change subject: ms: store in bts->ms_list during alloc/destroy of ms object
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/32346
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Id53f8dfb9963366dd4b19a324615bbc83c4f23e7
Gerrit-Change-Number: 32346
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Apr 2023 17:50:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment