<p><a href="https://gerrit.osmocom.org/c/osmo-msc/+/15317">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c">File src/libmsc/msc_a.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/15317/3/src/libmsc/msc_a.c@1411">Patch Set #3, Line 1411:</a> <code style="font-family:monospace,monospace">but this static msgb saves the extra allocation</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;">I don't see how this can be a problem. None of the DTAP handling code is permitted to take ownership of the msgb.</p><p style="white-space: pre-wrap; word-wrap: break-word;">What we would do here is allocate a msgb, dispatch it to DTAP, then deallocate it right after that here in this function. Even if it were allocated, it would still not be possible to put it in a queue. Nothing would be gained AFAICT.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is the osmo-msc internal DTAP code, it is fully contained here: all handling of this msgb is in gsm_04_08.c. We're not talking about the general public or another osmocom library here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The premise of DTAP decoding is to keep the incoming buffer unchanged, essentially a 'const' by convention. It is the incoming data and it is not a good idea to want to modify it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Passing the msgb as 'const' is not necessary: even though this is a stack struct msgb, we are still free to write to it. We are not allowed to free it, but none of the DTAP code is allowed to free msgb passed to it -- it would lead to a double free.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you guys still insist, this can become a dynamic allocation, but I am convinced that this patch is completely fine as it is now. The data is there, no need to allocate.</p><p style="white-space: pre-wrap; word-wrap: break-word;">[BTW, talking of const msgb... I never added the 'const' to msgb args because the legacy gsm0408_* code neglected that. I would gladly change it to const, now that we have the time for it. But we do typically write l3h and cb items into the msgb, i.e. keep state for later in the code path. Also the msgb API from libosmocore often takes non-const msgb even for read-only API (e.g. msgb_get_u8()), which would need a lot of casting to non-const.]</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-msc/+/15317">change 15317</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-msc/+/15317"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-msc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d </div>
<div style="display:none"> Gerrit-Change-Number: 15317 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 02 Sep 2019 23:35:47 +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: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>