neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16205 )
Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name ...................................................................... Patch Set 29: (1 comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c File src/gsupclient/gsup_req.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@142 PS26, Line 142: osmo_gsup_req_free > Actually I want to avoid bugs by including the free everywhere implicitly. […] Taking another look... (I notice that osmo_gsup_make_response() also changes the session_state in the response message depending on the final_response flag. So I need a final_response flag anyway, even if the free were a separate call.) Having the free() implicitly as soon as there is a final response enforces a request-response relation in GSUP. Some background... { The typical case is: rx GSUP, tx response. With the new osmo_gsup_req we can easily expand to: rx GSUP, wait async, tx response. Only few GSUP constructs go beyond a 1:1 request-response match. But some have a session or more negotiation may happen before the initial request is completed, so the final_response flag adds: rx GSUP, wait async, [tx other, wait async, rx other...,] tx final response. By definition, all of these either end in a final response or an error. So I want to tie the lifetime of the osmo_gsup_req to the req -> resp/err cycle, to avoid memory leaks and enforce the GSUP request-response model. } We could argue that we may not want to enforce a req -> resp/err like this? Personally I don't like very much that the final_response is just a little 'true'/'false' that is easy to miss when reading code; but I also don't want to multiply nr of function signatures by two by adding nonfinal functions (osmo_gsup_req_respond{_nonfinal}, osmo_gsup_req_respond_msgt{_nonfinal}). Making the true/false flag more obvious could be done by an enum value (osmo_gsup_req_respond(GSUP_RESP_FINAL, ...)). An enum value also opens the door to different ways to do a GSUP response in the future, if ever needed (like a "final" response but doesn't free??). mem leaks: using msgb(), we often have/had hidden memleaks. With an implicit free we may be able to mostly avoid the entire class of gsup_req memleaks. I think I would implement the GSUP_RESP_[NON]FINAL enum value now and keep the free() implicit. Seems to be the most future proof yet most bug avoiding option. But since changing this ripples across multiple files in multiple patches, I'd like more opinions before I change anything. If no opinions, I guess it should just remain unchanged...? -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 Gerrit-Change-Number: 16205 Gerrit-PatchSet: 29 Gerrit-Owner: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <axilira...@gmail.com> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Tue, 28 Apr 2020 13:50:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <axilira...@gmail.com> Comment-In-Reply-To: neels <nhofm...@sysmocom.de> Gerrit-MessageType: comment
