Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
laforge 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 31: Code-Review+2 -- 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: 31 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 01 May 2020 14:33:21 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello fixeria, pespin, laforge, osmith, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#31). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** cni_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in cni_peer_id.h and cni_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am A src/gsupclient/gsup_req.c A src/
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello fixeria, pespin, laforge, osmith, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#30). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** cni_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in cni_peer_id.h and cni_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am A src/gsupclient/gsup_req.c A src/
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
laforge has removed Jenkins Builder from 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 .. Removed reviewer Jenkins Builder with the following votes: * Verified+1 by Jenkins Builder (102) -- 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 Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: deleteReviewer
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
laforge 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 > Taking another look... […] I don't have a strong opinion, but a slight preference to keep the patch as-is now. -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 28 Apr 2020 16:43:17 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: osmith
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Tue, 28 Apr 2020 13:50:11 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria
Comment-In-Reply-To: neels
Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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: Code-Review-1 (still unresolved issues, see above. Just fixed the merge conflict.) -- 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 Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Tue, 28 Apr 2020 12:56:30 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello fixeria, pespin, laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#29). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am A src/gsupclie
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
fixeria 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 28: (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@110 PS26, Line 110: msgb_free(req->msg); > Have we done this elsewhere before? Not yet, but I don't see any problems. > But msgb are a bit different, AFAIU they're all part of a separate msgb > talloc context It's just a context msgb_alloc() uses by default. There is also msgb_alloc_c(). -- 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: 28 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Thu, 30 Jan 2020 07:38:43 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 28: Code-Review-1 (1 comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/28/include/osmocom/gsupclient/ipa_name.h File include/osmocom/gsupclient/ipa_name.h: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/28/include/osmocom/gsupclient/ipa_name.h@31 PS28, Line 31: uint8_t val[128]; this stuff here is still unresolved. It was conceived with the idea in mind that it would contain arbitrary data. Seems to not make sense after all. need to decide how to do this then to cosmetically ripple through all those patches again to apply what was decided (but I'm not doing that right now) -- 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: 28 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 29 Jan 2020 12:59:20 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 28: (1 comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c File src/gsup_server.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@444 PS26, Line 444: osmo_gsup_create_insert_subscriber_data_msg > I don't see any functional changes to this function, you're basically > converting tabs to spaces... thx -- what the heck happened here !? -- 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: 28 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 29 Jan 2020 12:54:07 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello fixeria, pespin, laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#28). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am A src/gsupclie
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello fixeria, pespin, laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#27). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am A src/gsupclie
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 26: (15 comments) In general, I wish that the emotional load of applying these changes would not rest only on my shoulders. Reviewers keep indicating how much time they spent on reading and how bad it is to read this code, but please acknowledge that we are writing these patches to accomodate the needs of rural communities, and that we are implementing these things as Osmocom, not as neels vs. the rest. I am trying to find the best path with the best intentions. The review that took an hour to write took me one and a half hours to read and respond/apply. I don't know how many hours I am spending on reading review comments, how many times a week, it is ridiculous to point it out, since the numbers hardly compare to the time I am spending on this side, not even on separating the patches, but on explaining how that is hard. Otherwise, thanks to fixeria for the errors found, I really welcome that! https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c File src/gsup_server.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@82 PS26, Line 82: osmo_gsup_message_to_str_c > Do we really need such detailed logging? Not sure if this supposed to happen > too often. […] "unable to encode" is probably not ever even once going to happen in practice https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@89 PS26, Line 89: osmo_gsup_message_to_str_c > Same here. Chances are that this message would confuse the end user... […] "unable to send" is not something I expect to actually happen. This is coding wise the easiest to provide context to the log message. The alternative is '"%s %s", response->imsi, osmo_gsup_message_type_name(...)' (annoying to type and error prone, so I am trying to get away from that) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@104 PS26, Line 104: GSUP message > I think you can print the message type since osmo_gsup_req_new() decodes it > for you. the point of this message is the peer routing, i.e. one layer removed from individual subscribers, hence not including the message type https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@108 PS26, Line 108: osmo_gsup_req_respond_msgt(req, OSMO_GSUP_MSGT_ROUTING_ERROR, true); ^ this one frees the gsup_req, since it passes final=true https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@109 PS26, Line 109: return NULL; > Ok, now (after spending an hour reading the code) I know that > osmo_gsup_req_respond_msgt() can also […] indeed https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c File src/gsupclient/gsup_client.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c@294 PS26, Line 294: ipa_client_conn_create2 > Should be easy to submit this as a separate change. […] ack 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@110 PS26, Line 110: msgb_free(req->msg); > BTW: you could just do talloc_steal(req, msg) in osmo_gsup_req_new(). […] oh, you mean change the talloc hierarchy so that the msgb is a child of the gsup req? That's a nice idea. But msgb are a bit different, AFAIU they're all part of a separate msgb talloc context; Have we done this elsewhere before? https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@129 PS26, Line 129: !req->send_response_cb > Do we expect to have an instance of osmo_gsup_req without the response cb? If > not, just add an asser […] I want to not enforce it, it's not a problem at all to keep it conditional. I could invent scenarios, like, you just want to send PurgeMS Requests and don't care about the responses, or you're writing a unit test, but in the end it's just benefiting flexible implementations if it's trivial. https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@142 PS26, Line 142: osmo_gsup_req_free > I would better avoid doing the cleanup here and leave it up to the caller. […] Actually I want to avoid bugs by including the free everywhere implicitly. It sort of crept in later that some code paths don't want to free... I also had two separate function signatures at first: the normal osmo_gsup_req_respond() and a separate osmo_gsup_req_respond_nonfree(). Maybe we should get back to that to make it more obvious? https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c File src/gsupclient/ipa_name.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c@90 PS23, Line 90: return osmo_escape_str_c(OTC_SELECT, (char*)ipa_name->val, len); > 1- I mi
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 26: (meh, the new gerrit UI keeps making me miss comments. I don't get it, some comments are plain visible, others are greyed out in a single line and look like jenkins cruft.) > 1- I didn't say it was easy, yet is something really desirable. > 3- It's not only about reviewing for merge. It's a problem about discussing > each change. > It's a problem when reading the git log. > It's a problem when bisecting. > It's a problem when understanding why and how stuff was changed, and the > implications it had. > It's a problem when reverting. I still agree completely. We don't need to debate the above points at all. Instead we need to discuss the practical tradeoff, which is quite debatable. At least for sponsored work, a large part of the question is: How much budget do you want to spend on it and what is the actual practical positive impact you expect from it? So let's say we do spend another week on separating this patch into three different ones. Then each of this infrastructure implemented on its own will need other (probably weird/ad hoc) inventions to make the intermediate code work. Instead of one large change that makes sense, you get three changes that each take a large step into weird no-mans-land of ad hoc solutions. I doubt that this would help during bisecting, likely some bugs appear in-between the patches to be fixed again right away, and reviewers / log readers need to understand ad-hoc shims just to see them being removed again right in the next patch. That's what I saw coming out of separating, so I stopped that. I really mean that you should try to separate it if you have the time, then it will become obvious how it is not trivial. This seems to me a discussion whether we want large changes at all. It is not possible to trivially change osmo-hlr to achieve a big change like this. BTW, we have used this code at 36c3 in december. OsmoHLR hasn't made any problems. > 2- I agree with keeping ipa-name as a null-terminated string. I started out thinking that we would use this struct for all kinds of identification, i.e. also as Global Title buffer, i.e. that we need to regard it as opaque blob of data. The code now seems to go a different direction, so I guess I don't know which way to take this. I don't want to pass it as an allocated string to avoid ownership. But keeping a separate length seems unnecessary... We did do a bunch of work previously to handle it as opaque blob, explicitly allowing both with nul and without nul; now I could use some help deciding what it should actually be... -- 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: 26 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 29 Jan 2020 11:44:16 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
fixeria 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 26: (1 comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c File src/gsup_server.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@109 PS26, Line 109: return NULL; > Shouldn't we call osmo_gsup_req_free() here? Ok, now (after spending an hour reading the code) I know that osmo_gsup_req_respond_msgt() can also free() req. So we should not. -- 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: 26 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 29 Jan 2020 01:20:45 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
fixeria 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 26: Code-Review-1
(18 comments)
It took me about one hour to review this change. And now I noticed 1/2 in the
title. Some of my comments are critical (e.g. memleaks), some are not. I don't
like something that was supposed to be merged as a feature changes the code
base that much. Most of the handover-related bomb changes to OsmoMSC I had a
pleasure to review made the code more flexible, while this one makes it more
complicated. Especially that part with const req->gsup (decoded message) and
the way how instances of osmo_gsup_req are supposed to be free()d.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c
File src/gsup_server.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@82
PS26, Line 82: osmo_gsup_message_to_str_c
Do we really need such detailed logging? Not sure if this supposed to happen
too often. One can always use gdb to inspect the contents of structures...
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@89
PS26, Line 89: osmo_gsup_message_to_str_c
Same here. Chances are that this message would confuse the end user... Printing
just a message type would be enough IMHO.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@104
PS26, Line 104: GSUP message
I think you can print the message type since osmo_gsup_req_new() decodes it for
you.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@109
PS26, Line 109: return NULL;
Shouldn't we call osmo_gsup_req_free() here?
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@428
PS26, Line 428:
ws
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@444
PS26, Line 444: osmo_gsup_create_insert_subscriber_data_msg
I don't see any functional changes to this function, you're basically
converting tabs to spaces...
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c
File src/gsupclient/gsup_client.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c@294
PS26, Line 294: ipa_client_conn_create2
Should be easy to submit this as a separate change. You're basically fixing a
deprecation warning here...
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@110
PS26, Line 110: msgb_free(req->msg);
BTW: you could just do talloc_steal(req, msg) in osmo_gsup_req_new(). As a
bonus, we would get cleaner hierarchy in the talloc reports.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@129
PS26, Line 129: !req->send_response_cb
Do we expect to have an instance of osmo_gsup_req without the response cb? If
not, just add an assert(send_response_cb != NULL) to osmo_gsup_req_new().
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@142
PS26, Line 142: osmo_gsup_req_free
I would better avoid doing the cleanup here and leave it up to the caller. Yes,
it helps to avoid code duplication, but at the same time makes it harder to
spot bugs. It's just one line of code after all.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@165
PS26, Line 165: osmo_gsup_req_free
Same here.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c
File src/hlr.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c@447
PS26, Line 447: req->gsup.source_name[0]
The source_name is a pointer (not a buffer) and it can be NULL. Remove [0].
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c@448
PS26, Line 448: destination_name
Same.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c
File src/hlr_ussd.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@255
PS26, Line 255: return rc;
memleak, add talloc_free(msg)
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@284
PS26, Line 284: struct osmo_gsup_message resp = {
: .message_type = gsup_msg_type,
: .session_id = ss->session_id,
: };
I am sorry, but what's the point of this change? Just to make everything in the
code use your favorite way of initializing structures? Don't get me wrong, I am
not against using this in the new code, or where you're actually changing
something (e.g. adding new fields). But this patch is already quite big, and
such changes don't make it easier to read...
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@474
PS26, Line 474: sending might modify some (routing related?) parts
That's odd, I would even say unacceptable. A "*_send" function is not supposed
to change anything by definition and should accept a const
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
osmith 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 26: Code-Review+1 I agree with Pau that it's hard to review this patch properly, due to its size. But I also understand Neels reasoning for not spending the time on splitting it up further, and the even larger inter-msc handover related refactoring was accepted too. >From quickly skimming through this patch, I did not find any issues. It looks like all issues that Pau found are resolved. A new OsmoHLR release was also done recently. Pau, can you take another look at this patch (and if possible also follow-up patches), so we can finish up merging these D-GSM patches? -- 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: 26 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-CC: pespin Gerrit-Comment-Date: Tue, 28 Jan 2020 13:04:12 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
osmith has uploaded a new patch set (#25) to the change originally created by neels. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16205 ) Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am M src/gsupclient/gsup_client.c A src/gsupclient/gsup_req.c A src/gsupclie
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#24). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am M src/gsupclient/gsup_client.c
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 23: (2 comments) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c File src/db.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c@25 PS23, Line 25: #include > dev cruft indeed Done https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c@426 PS23, Line 426: osmo_gsup_message_type_name((gsup)->message_type), \ > Fixing () in the define could be a separate commit. Done -- 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: 23 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-CC: pespin Gerrit-Comment-Date: Thu, 09 Jan 2020 17:52:43 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
pespin 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 23: (1 comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c File src/gsupclient/ipa_name.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c@90 PS23, Line 90: return osmo_escape_str_c(OTC_SELECT, (char*)ipa_name->val, len); > If I add a _c i'd also have to pass OTC_SELECT everywhere, which would > probably be the proper way. […] 1- I might not understand it correctly, but isn't it find also allocating a "" string? It's a 1 byte buffer with '\0' on it right? 2- I'd really leave it up to the program to use OTC_SELECT or not, specially since it's only being used for logging purposes... As you know, I'm not really happy about this OTC_SELECT thing, and I specially dislike seeing it used for stuff which can be done in other ways easily too (for instance by passing a stack buffer as a param). -- 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: 23 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-CC: pespin Gerrit-Comment-Date: Thu, 09 Jan 2020 14:59:30 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 23: (4 comments) https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c File src/db.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c@25 PS23, Line 25: #include > Non related at all, must go into a separate patch (if needed at all). dev cruft indeed https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsup_router.c File src/gsup_router.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsup_router.c@99 PS23, Line 99: int gsup_route_add_ipa_name(struct osmo_gsup_conn *conn, const struct osmo_ipa_name *ipa_name) > and with this type of structs we end up doing like this function, simply > adding extra layers. I find passing a val and separate length around very annoying, and it is also error prone. It adds extra lines of code and micro-code-dup all over the place. We should have kept a single struct from the start. I'd rather drop the gsup_route_add() version. https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c File src/gsupclient/ipa_name.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c@90 PS23, Line 90: return osmo_escape_str_c(OTC_SELECT, (char*)ipa_name->val, len); > So you are forcing the app using the library to enable the otc_select in this > case? SHouldn't the fu […] If I add a _c i'd also have to pass OTC_SELECT everywhere, which would probably be the proper way. That would also imply not being able to return a "" because the returned value always needs to be allocated. I kind of dislike writing OTC_SELECT in every LOGP(), so I thought I might get away with this more convenient combination; all current GSUP client programs use osmo_select_main_ctx()... Is that acceptable? https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c@777 PS23, Line 777: osmo_select_main_ctx(0); > Is this line change only needed to print stuff more easily? I think so, yes. It's a valid reason, too :) -- 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: 23 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-CC: pespin Gerrit-Comment-Date: Thu, 09 Jan 2020 14:53:01 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
pespin 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 23: 1- I didn't say it was easy, yet is something really desirable. 2- I agree with keeping ipa-name as a null-terminated string. Simply add checks in case it's not upon receiving, but afaik it's not null terminated only on really old code of ours (and maybe nanobts?) which anyway is not being used here, and which was fixed a while ago to be null-terminated. 3- It's not only about reviewing for merge. It's a problem about discussing each change. It's a problem when reading the git log. It's a problem when bisecting. It's a problem when understanding why and how stuff was changed, and the implications it had. It's a problem when reverting. -- 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: 23 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-CC: pespin Gerrit-Comment-Date: Thu, 09 Jan 2020 14:48:32 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 23:
(1 comment)
@pespin, you keep saying that separating is easy, and I keep trying to explain
how it isn't: this patch has tight interdependence between the new items. You
are welcome to try to separate. I have spent hours and days separating patches
already, which did make sense in many places. I understand the point very much.
But it is still necessary to review the combined result, and it makes little
sense to spend more time on creating intermediate versions of osmo-hlr that
will never be used. I know the review is harder than it should be, but
separating would probably take longer than reviewing as-is. does that make
sense?
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h
File include/osmocom/gsupclient/ipa_name.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h@29
PS23, Line 29: struct osmo_ipa_name {
> I'm not really liking this kind of adhoc structures to keep a string
> specially since they are presen […]
the IPA name is a problem child, it is not entirely clear whether it is a
string or arbitrary bytes with a length.
The discussion was that this should also support global titles (that could
contain zero bytes),
and I assumed that we need to make all code paths treat it as binary data.
However, many code paths expect it to be nul terminated, especially the hlr.db
stores as VARCHAR.
And then in code review quite recently, the request was to add a union that
allows adding a global title separately, which I did (in the next patch "2/2"
after this one).
In retrospect it might make sense to snap ipa_name back to a plain char[] that
is always nul terminated?
Drop the separate len indicator? I think we should resolve this question before
merging, feels like it indeed doesn't make sense like this.
--
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: 23
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: osmith
Gerrit-CC: pespin
Gerrit-Comment-Date: Thu, 09 Jan 2020 14:23:52 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
pespin 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 23:
(7 comments)
IMHO too many things going on in this commit which are not really related and
which could have been easily splitted and discussed separately, like the use of
ipa_name struct, te _req*, and other fixes in the way simply pulled in in the
same patch.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h
File include/osmocom/gsupclient/ipa_name.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/gsupclient/ipa_name.h@29
PS23, Line 29: struct osmo_ipa_name {
I'm not really liking this kind of adhoc structures to keep a string specially
since they are presented in a public header of shared library, meaning we'll
need to keep with it later on...
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/hlr/gsup_router.h
File include/osmocom/hlr/gsup_router.h:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/include/osmocom/hlr/gsup_router.h@31
PS23, Line 31: int osmo_gsup_send_to_ipa_name(struct osmo_gsup_server *gs,
const struct osmo_ipa_name *ipa_name, struct msgb *msg);
Update TODO_RELEASE explaining there's new API.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c
File src/db.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/db.c@25
PS23, Line 25: #include
Non related at all, must go into a separate patch (if needed at all).
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsup_router.c
File src/gsup_router.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsup_router.c@99
PS23, Line 99: int gsup_route_add_ipa_name(struct osmo_gsup_conn *conn, const
struct osmo_ipa_name *ipa_name)
and with this type of structs we end up doing like this function, simply adding
extra layers.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c
File src/gsupclient/ipa_name.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c@90
PS23, Line 90: return osmo_escape_str_c(OTC_SELECT, (char*)ipa_name->val, len);
So you are forcing the app using the library to enable the otc_select in this
case? SHouldn't the function have some kind of suffix to mark it as so (_c) and
explain that in the API doc?
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c
File src/hlr.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c@426
PS23, Line 426: osmo_gsup_message_type_name((gsup)->message_type),
\
Fixing () in the define could be a separate commit.
https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/hlr.c@777
PS23, Line 777: osmo_select_main_ctx(0);
Is this line change only needed to print stuff more easily?
--
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: 23
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: neels
Gerrit-Reviewer: osmith
Gerrit-CC: pespin
Gerrit-Comment-Date: Wed, 08 Jan 2020 13:14:29 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
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 22: (3 comments) This actually changes coding of the Routing Error message: So far a Routing Error sent back Source Name and Destination Name unchanged. In a setup with possibly a GSUP proxy between request and response, that is very cumbersome. It makes much more sense to put the requesting source in Destination Name, so that the error is implicitly routed back to the entity that requested the unreachable peer. Related: "add version IE?" https://osmocom.org/issues/4333 Places that introduce the "swap" of Destination and Source are... https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/gsup_server.c File src/gsup_server.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/gsup_server.c@108 PS22, Line 108: osmo_gsup_req_respond_msgt(req, OSMO_GSUP_MSGT_ROUTING_ERROR, true); here https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/gsupclient/gsup_req.c File src/gsupclient/gsup_req.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/gsupclient/gsup_req.c@85 PS22, Line 85: osmo_gsup_req_respond_msgt(req, OSMO_GSUP_MSGT_ROUTING_ERROR, true); here https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/hlr.c File src/hlr.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16205/22/src/hlr.c@474 PS22, Line 474: .source_name_len = gsup->destination_name_len, here -- 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: 22 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Comment-Date: Mon, 16 Dec 2019 14:06:57 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
laforge 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 22: Code-Review+1 -- 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: 22 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Comment-Date: Sat, 14 Dec 2019 17:32:46 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
osmith has uploaded a new patch set (#22) to the change originally created by neels. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16205 ) Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db.c M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am M src/gsupclient/gsup_client.c A src/gsupclient/gsup_req.c A s
Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name
Hello osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hlr/+/16205 to look at the new patch set (#21). Change subject: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name .. 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name These are seemingly orthogonal changes in one patch, because they are in fact sufficiently intertwined that we are not willing to spend the time to separate them. They are also refactoring changes, unlikely to make sense on their own. ** lu_fsm: Attempting to make luop.c keep state about incoming GSUP requests made me find shortcomings in several places: - since it predates osmo_fsm, it is a state machine that does not strictly enforce the order of state transitions or the right sequence of incoming events. - several places OSMO_ASSERT() on data received from the network. - modifies the subscriber state before a LU is accepted. - dead code about canceling a subscriber in a previous VLR. That would be a good thing to actually do, which should also be trivial now that we record vlr_name and sgsn_name, but I decided to remove the dead code for now. To both step up the LU game *and* make it easier for me to integrate osmo_gsup_req handling, I decided to create a lu_fsm, drawing from my, by now, ample experience of writing osmo_fsms. ** osmo_gsup_req: Prepare for D-GSM, where osmo-hlr will do proxy routing for remote HLRs / communicate with remote MSCs via a proxy: a) It is important that a response that osmo-hlr generates and that is sent back to a requesting MSC contains all IEs that are needed to route it back to the requester. Particularly source_name must become destination_name in the response to be able to even reach the requesting MSC. Other fields are also necessary to match, which were so far taken care of in individual numerous code paths. b) For some operations, the response to a GSUP request is generated asynchronously (like Update Location Request -> Response, or taking the response from an EUSE, or the upcoming proxying to a remote HLR). To be able to feed a request message's information back into the response, we must thus keep the request data around. Since struct osmo_gsup_message references a lot of external data, usually with pointers directly into the received msgb, it is not so trivial to pass GSUP message data around asynchronously, on its own. osmo_gsup_req is the combined solution for both a and b: it keeps all data for a GSUP message by taking ownership of the incoming msgb, and it provides an explicit API "forcing" callers to respond with osmo_gsup_req_respond(), so that all code paths trivially are definitely responding with the correct IEs set to match the request's routing (by using osmo_gsup_make_response() recently added to libosmocore). Adjust all osmo-hlr code paths to use *only* osmo_gsup_req to respond to incoming requests received on the GSUP server (above LU code being one of them). In fact, the same should be done on the client side. Hence osmo_gsup_req is implemented in a server/client agnostic way, and is placed in libosmo-gsupclient. As soon as we see routing errors in complex GSUP setups, using osmo_gsup_req in the related GSUP client is likely to resolve those problems without much thinking required beyond making all code paths use it. libosmo-gsupclient is hence added to osmo-hlr binary's own library dependencies. It would have been added by the D-GSM proxy routing anyway, we are just doing it a little sooner. ** gsup_peer_id.c / osmo_ipa_name: We so far handle an IPA unit name as pointer + size, or as just pointer with implicit talloc size. To ease working with GSUP peer identification data, I require: - a non-allocated storage of an IPA Name. It brings the drawback of being size limited, but our current implementation is anyway only able to handle MSC and SGSN names of 31 characters (see struct hlr_subscriber). - a single-argument handle for IPA Name, - easy to use utility functions like osmo_ipa_name_to_str(), osmo_ipa_name_cmp(), and copying by simple assignment, a = b. Hence this patch adds a osmo_ipa_name in gsup_peer_id.h and gsup_peer_id.c. Heavily used in LU and osmo_gsup_req. Depends: libosmocore Id9692880079ea0f219f52d81b1923a76fc640566 Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 --- M configure.ac M include/Makefile.am A include/osmocom/gsupclient/gsup_req.h A include/osmocom/gsupclient/ipa_name.h M include/osmocom/hlr/Makefile.am M include/osmocom/hlr/db.h M include/osmocom/hlr/gsup_router.h M include/osmocom/hlr/gsup_server.h M include/osmocom/hlr/hlr.h M include/osmocom/hlr/hlr_ussd.h M include/osmocom/hlr/logging.h A include/osmocom/hlr/lu_fsm.h D include/osmocom/hlr/luop.h M src/Makefile.am M src/db.c M src/db_hlr.c M src/gsup_router.c M src/gsup_send.c M src/gsup_server.c M src/gsupclient/Makefile.am M src/gsupclient/gsup_client.
