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.