<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p>Patch set 26:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205">View Change</a></p><p>18 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c">File src/gsup_server.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@82">Patch Set #26, Line 82:</a> <code style="font-family:monospace,monospace">osmo_gsup_message_to_str_c</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@89">Patch Set #26, Line 89:</a> <code style="font-family:monospace,monospace">osmo_gsup_message_to_str_c</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here. Chances are that this message would confuse the end user... Printing just a message type would be enough IMHO.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@104">Patch Set #26, Line 104:</a> <code style="font-family:monospace,monospace">GSUP message </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you can print the message type since osmo_gsup_req_new() decodes it for you.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@109">Patch Set #26, Line 109:</a> <code style="font-family:monospace,monospace">return NULL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Shouldn't we call osmo_gsup_req_free() here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@428">Patch Set #26, Line 428:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ws</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsup_server.c@444">Patch Set #26, Line 444:</a> <code style="font-family:monospace,monospace">osmo_gsup_create_insert_subscriber_data_msg</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see any functional changes to this function, you're basically converting tabs to spaces...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c">File src/gsupclient/gsup_client.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_client.c@294">Patch Set #26, Line 294:</a> <code style="font-family:monospace,monospace">ipa_client_conn_create2</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should be easy to submit this as a separate change. You're basically fixing a deprecation warning here...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c">File src/gsupclient/gsup_req.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@110">Patch Set #26, Line 110:</a> <code style="font-family:monospace,monospace">msgb_free(req->msg);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@129">Patch Set #26, Line 129:</a> <code style="font-family:monospace,monospace">!req->send_response_cb</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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().</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@142">Patch Set #26, Line 142:</a> <code style="font-family:monospace,monospace">osmo_gsup_req_free</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/gsupclient/gsup_req.c@165">Patch Set #26, Line 165:</a> <code style="font-family:monospace,monospace">osmo_gsup_req_free</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c">File src/hlr.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c@447">Patch Set #26, Line 447:</a> <code style="font-family:monospace,monospace">req->gsup.source_name[0]</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The source_name is a pointer (not a buffer) and it can be NULL. Remove [0].</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr.c@448">Patch Set #26, Line 448:</a> <code style="font-family:monospace,monospace">destination_name</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c">File src/hlr_ussd.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@255">Patch Set #26, Line 255:</a> <code style="font-family:monospace,monospace">return rc;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">memleak, add talloc_free(msg)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@284">Patch Set #26, Line 284:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct osmo_gsup_message resp = {<br>             .message_type = gsup_msg_type,<br>                .session_id = ss->session_id,<br>      };<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@474">Patch Set #26, Line 474:</a> <code style="font-family:monospace,monospace">sending might modify some (routing related?) parts</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/src/hlr_ussd.c@499">Patch Set #26, Line 499:</a> <code style="font-family:monospace,monospace">return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">No need, we return 0 below anyway.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/tests/testsuite.at">File tests/testsuite.at:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/26/tests/testsuite.at@22">Patch Set #26, Line 22:</a> <code style="font-family:monospace,monospace">experr</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This file is empty, you could just use 'ignore'.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205">change 16205</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-hlr </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I3a8dff3d4a1cbe10d6ab08257a0138d6b2a082d9 </div>
<div style="display:none"> Gerrit-Change-Number: 16205 </div>
<div style="display:none"> Gerrit-PatchSet: 26 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 29 Jan 2020 01:16:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>