Change in osmo-hlr[master]: 1/2: refactor: add and use lu_fsm, osmo_gsup_req, osmo_ipa_name

historical

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 might not understand it correctly, but isn't it find also allocating a "" string? It's a 1 byte […]
re 1: sure, we can return an allocated "" too...

re 2: I really like to get away from stack buffers, especially since we keep creating new stack buffers for every function. It silently increases the mem footprint of the program by mostly unused relatively arbitrary string buffers. The alternative would be to create a single stack buffer for all, but then we run into problems printing more than one info in a LOGP(). I had in the beginning proposed a solution that assigned bits of a single large stack buffer to each function call, but it was overruled by the select ctx ... I don't like having to pass OTC_SELECT as arg, but that's about everything I don't like about it; in general I think it is a pretty elegant solution.

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].
oh right... so quirky how gsup_message has some pointers and some buffers

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)
thx

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 favori […]
indeed

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. […]
the most important job of osmo_gsup_req is to ensure that all GSUP routing related bits are set correctly. We could copy the gsup_message struct every time in the gsup req api, but callers usually create a struct on the stack to set some few bits and can pass this into gsup_req to complete the message.

https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/tests/testsuite.at 
File tests/testsuite.at:

https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/tests/testsuite.at@22 
PS26, Line 22: experr
> This file is empty, you could just use 'ignore'.
not if i want to verify that stderr is empty :P

-- 
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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 29 Jan 2020 12:25:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <axilirator at gmail.com>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200129/71189941/attachment.htm>