<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Otherwise, thanks to fixeria for the errors found, I really welcome that!</p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205">View Change</a></p><p>15 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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do we really need such detailed logging? Not sure if this supposed to happen too often. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">"unable to encode" is probably not ever even once going to happen in practice</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Same here. Chances are that this message would confuse the end user... […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">"unable to send" is not something I expect to actually happen.<br>This is coding wise the easiest to provide context to the log message. The alternative is<br>'"%s %s", response->imsi, osmo_gsup_message_type_name(...)'<br>(annoying to type and error prone, so I am trying to get away from that)</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think you can print the message type since osmo_gsup_req_new() decodes it for you.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">the point of this message is the peer routing, i.e. one layer removed from individual subscribers, hence not including the message type</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@108">Patch Set #26, Line 108:</a> <code style="font-family:monospace,monospace">                      osmo_gsup_req_respond_msgt(req, OSMO_GSUP_MSGT_ROUTING_ERROR, true);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">^ this one frees the gsup_req, since it passes final=true</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ok, now (after spending an hour reading the code) I know that osmo_gsup_req_respond_msgt() can also  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">indeed</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should be easy to submit this as a separate change. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">BTW: you could just do talloc_steal(req, msg) in osmo_gsup_req_new(). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh, you mean change the talloc hierarchy so that the msgb is a child of the gsup req? That's a nice idea.<br>But msgb are a bit different, AFAIU they're all part of a separate msgb talloc context;<br>Have we done this elsewhere before?</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do we expect to have an instance of osmo_gsup_req without the response cb? If not, just add an asser […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I want to not enforce it, it's not a problem at all to keep it conditional.<br>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.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would better avoid doing the cleanup here and leave it up to the caller. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually I want to avoid bugs by including the free everywhere implicitly.<br>It sort of crept in later that some code paths don't want to free...</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16205/23/src/gsupclient/ipa_name.c">File src/gsupclient/ipa_name.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/23/src/gsupclient/ipa_name.c@90">Patch Set #23, Line 90:</a> <code style="font-family:monospace,monospace">      return osmo_escape_str_c(OTC_SELECT, (char*)ipa_name->val, len);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">1- I might not understand it correctly, but isn't it find also allocating a "" string? It's a 1 byte […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">re 1: sure, we can return an allocated "" too...</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The source_name is a pointer (not a buffer) and it can be NULL. Remove [0].</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh right... so quirky how gsup_message has some pointers and some buffers</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">memleak, add talloc_free(msg)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">thx</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I am sorry, but what's the point of this change? Just to make everything in the code use your favori […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">indeed</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">That's odd, I would even say unacceptable. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This file is empty, you could just use 'ignore'.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">not if i want to verify that stderr is empty :P</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 12:25:50 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>