<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/16189">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/16189/3/src/gsm/gsup.c">File src/gsm/gsup.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/libosmocore/+/16189/3/src/gsm/gsup.c@910">Patch Set #3, Line 910:</a> <code style="font-family:monospace,monospace"> * Note: after calling this function, fields in the reply may reference the same memory as rx and are not deep-copied.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ack</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">that is what the osmo_gsup_req is for: it keeps the incoming message's state.<br>This non-deep copying of gsup messages is happening all over the osmo-hlr and gsup client code base.<br>We always keep the msgb around until the contents no longer matter.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Note that a struct osmo_gsup_message is so far never dynamically allocated, anywhere.<br>So if we wanted to deep-copy with dynamic allocations, we would open up a whole new refactoring slur all over the place. It's one of the things we should have designed better from the start, too late now in my opinion.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The solution to allow caching (decoded) GSUP messages also asynchronously is the osmo_gsup_req API added in osmo-hlr.git, in libosmo-gsupclient. So far we never kept decoded GSUP asynchronously, but will do for D-GSM, and btw also strictly require this in order for proxy routing to be guaranteed to work: to copy the right routing information back to the response.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Given that it's close to the IE layer, I thought here is the best place, but I'm going to move this into osmo_gsup_req now. You (I?) have just convinced me.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/16189/3/src/gsm/gsup.c@943">Patch Set #3, Line 943:</a> <code style="font-family:monospace,monospace">    if (!reply->imsi[0])</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ack</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">We almost nowhere do explicit '\0' char matching. I used to do that everywhere some years ago, but the million places where we just use '!' made me lose interest. I'd prefer to spend my time differently than reworking many patches for cosmetics. Functional flaws are much nicer to discuss, like above.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/16189/3/src/gsm/gsup.c@985">Patch Set #3, Line 985:</a> <code style="font-family:monospace,monospace">osmo_gsup_message_name_buf</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">_stringify()? _tostr() _sprint() ? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I do see what you mean.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(for me '_name' turned out to be the API naming for "convert to human readable string", compare also OSMO_NAME_C_IMPL(), which then could have been OSMO_STR_C_IMPL() instead... I think we can still rename it, not released yet, is it? I'm fairly sure there are many places where I called functions foo_name even though they produce more than just a short name, so keep an eye out. OTOH this is another minor cosmetic up for interpretation...)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/16189">change 16189</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/libosmocore/+/16189"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Id9692880079ea0f219f52d81b1923a76fc640566 </div>
<div style="display:none"> Gerrit-Change-Number: 16189 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </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: Thu, 28 Nov 2019 21:00:30 +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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>