Attention is currently required from: fixeria, laforge, pespin. neels 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 4: Code-Review-2 (5 comments) Patchset: PS4: I disagree with this patch, because you are mixing the "there is a free() call in the source" with "it is not freed anywhere". In osmo-msc, you need to grep for osmo_fsm_inst_term instead. The memory management in osmo-msc is far from trivial, and it took a good amount of time to make sure that each object tears down dependent objects, while not causing double frees or leaks. The decision that each FSM instance takes care of its own priv object is PIVOTAL to this design. So in short, you are starting at a big refactoring, which I disagree with. Commit Message: https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/008e3430_79910dfb?usp=email : 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. > It's logical, but it has drawbacks, like not being able to quickly figuring > out where the object is […] you may find that, but you have not convinced me yet. All FSMs that I implement have a priv object as the child of the fsm inst, hence this is the pattern i expect to see. It also accurately reflects that the FSM instance is responsible for deallocation. Could you describe an objective drawback, thx File src/libmsc/ran_peer.c: https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/21ab372d_6eef772c?usp=email : PS4, Line 551: .cleanup = ran_peer_fsm_cleanup, The fsm.cleanup function is an integral part of osmo-msc's object management, to handle corner cases gracefully. I am pretty sure that ran_peer needs a cleanup function to be resilient against failures. Beyond this, I subjectively prefer that we stick to the pattern that is used in the other FSM instances in osmo-msc. https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/82f9d6fb_c56735d6?usp=email : PS4, Line 81: void ran_peer_free(struct ran_peer *rp) AFAIR the idea is to instead call osmo_fsm_inst_term() https://gerrit.osmocom.org/c/osmo-msc/+/40632/comment/523d1d62_caa97f9a?usp=email : PS4, Line 129: static void ran_peer_discard_all_conns(struct ran_peer *rp) separate patch -- 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: 4 Gerrit-Owner: pespin <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Mon, 14 Jul 2025 13:26:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: fixeria <[email protected]>
