Attention is currently required from: fixeria, lynxis lazus, neels.
pespin has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-msc/+/40645?usp=email )
Change subject: vty: Get rid of unneeded iu_client vty config
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/40645/comment/ac48532b_aff0edc6?usp… :
PS1, Line 14: Th
> The COMMIT_MSG explains what the patch is doing, but not *why*. […]
Well it's all explained in OS#5487. osmo-msc doesn't really use iu_client since a while ago, this is more like remains of the previous removal.
I can write that in the commit message if you feel it's needed.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/40645?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I9cf83f2255e1e9aa83f3139b88ea81b2f5b686c3
Gerrit-Change-Number: 40645
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Mon, 14 Jul 2025 09:03:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: fixeria, neels.
pespin has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-msc/+/40632?usp=email )
Change subject: ran_peer: Add specific API to free object
......................................................................
Patch Set 2:
(4 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/35436821_5f040e5e?usp… :
PS2, Line 11: Swap the talloc tree parentship to have the fi be a child of the object,
: which simplifies tear down triggered by FSM cleanup.
> From the hierarchical point of view, I find it logical the way it is: the FSM instance's private dat […]
It's logical, but it has drawbacks, like not being able to quickly figuring out where the object is freed and hence it's entire lifecycle.
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/8e5ef0d8_162ca47d?usp… :
PS2, Line 14: Apparently the object is not being freed anywhere
> It's being free()d automatically by talloc, since it's allocated as a child of the fi: […]
I know it's freed by talloc automatically. Now, can you tell me where is the fsm instance being terminated/freed in the codebase? It's never freed anywhere. That's what I meant.
And yes, the fact that it's freed automatically by terminating/freeing the FSM doesn't really help precisely figuring out where this object is freed, because there's no specific API to grep for in the code.
File include/osmocom/msc/ran_peer.h:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/9da0baec_a9c6ed7b?usp… :
PS2, Line 109: ran_peer_discard_all_conns
> How is this related to this patch? Why exposing this API?
It's called by ran_peer_free(), so I need it declared beforehand.
It was already not set as static before, so I'm not really changing that here.
I can make it static and add a forward declaration in the .c file if you want.
File src/libmsc/ran_peer.c:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/9eeca83e_84219fe3?usp… :
PS2, Line 79: ran_peer_free
> You're adding this new API, but it's not being called anywhere. […]
Yeah well, these objects are not actually being freed anywhere in code (yet?). Still, it's good to have a clear lifecycle with all the logic so no bugs are introduced whenever somebody needs freeing them at some point later.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/40632?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I721de21a68a4e336ae508a995e3cfcca05d57efe
Gerrit-Change-Number: 40632
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 14 Jul 2025 09:01:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Attention is currently required from: lynxis lazus.
fixeria has posted comments on this change by lynxis lazus. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/40653?usp=email )
Change subject: gprs_gmm: fix endianess of GSM48_IE_GMM_PDP_CTX_STATUS
......................................................................
Patch Set 1:
(2 comments)
File src/sgsn/gprs_gmm.c:
https://gerrit.osmocom.org/c/osmo-sgsn/+/40653/comment/8f844203_71434c02?us… :
PS1, Line 1978: Create tlvp_val16be
`tlvp_val16be` is already there, did you mean `tlvp_val16le`?
https://gerrit.osmocom.org/c/osmo-sgsn/+/40653/comment/ffb5f194_299040c3?us… :
PS1, Line 1979: osmo_swab16(tlvp_val16be(&tp, GSM48_IE_GMM_PDP_CTX_STATUS));
I would avoid calling `tlvp_val16be()` and then swapping bytes.
`tlvp_val16be()` is calling `osmo_load16be()` internally, so you can do:
```
osmo_load16le(TLVP_VAL(&tp, GSM48_IE_GMM_PDP_CTX_STATUS))
```
--
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/40653?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: Ib9bee2f8d0b1f89986d15cf3ce6404ad76378c8c
Gerrit-Change-Number: 40653
Gerrit-PatchSet: 1
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Mon, 14 Jul 2025 08:50:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: lynxis lazus, neels, pespin.
fixeria has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-msc/+/40645?usp=email )
Change subject: vty: Get rid of unneeded iu_client vty config
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/40645/comment/d36bb56a_53bd1be1?usp… :
PS1, Line 14: Th
The COMMIT_MSG explains what the patch is doing, but not *why*.
What's the goal behind removing `iu_client.h`? This is a potentially breaking change, because it removes some VTY commands without introducing any stubs, etc.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/40645?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I9cf83f2255e1e9aa83f3139b88ea81b2f5b686c3
Gerrit-Change-Number: 40645
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Mon, 14 Jul 2025 08:43:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: neels, pespin.
fixeria has posted comments on this change by pespin. ( https://gerrit.osmocom.org/c/osmo-msc/+/40632?usp=email )
Change subject: ran_peer: Add specific API to free object
......................................................................
Patch Set 2: Code-Review-2
(4 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/e6ee89c6_0286c34b?usp… :
PS2, Line 11: Swap the talloc tree parentship to have the fi be a child of the object,
: which simplifies tear down triggered by FSM cleanup.
From the hierarchical point of view, I find it logical the way it is: the FSM instance's private data belongs to that instance and is a child of that instance.
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/d9143ed3_9b608f4a?usp… :
PS2, Line 14: Apparently the object is not being freed anywhere
It's being free()d automatically by talloc, since it's allocated as a child of the fi:
```
rp = talloc_zero(fi, struct ran_peer);
```
With your patch the `struct ran_peer` is no longer free()ed because nobody calls the new `ran_peer_free()` API.
File include/osmocom/msc/ran_peer.h:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/6ab938f3_57c642c6?usp… :
PS2, Line 109: ran_peer_discard_all_conns
How is this related to this patch? Why exposing this API?
File src/libmsc/ran_peer.c:
https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/e2fc2130_815dc69b?usp… :
PS2, Line 79: ran_peer_free
You're adding this new API, but it's not being called anywhere.
Nor it's declared in a public header.
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/40632?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I721de21a68a4e336ae508a995e3cfcca05d57efe
Gerrit-Change-Number: 40632
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 14 Jul 2025 08:29:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes