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

historical

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 pointer.

https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@499 
PS26, Line 499: return 0;
No need, we return 0 below anyway.

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'.

-- 
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 01:16:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200129/031ab256/attachment.htm>