<p style="white-space: pre-wrap; word-wrap: break-word;">I find the choice of name and return value a bit confusing:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">- recv_discard_msg() reads like it actually calls msgb_free() implicitly,<br>  but we still 'goto discard_msg', looks like we discard twice?</pre><ul><li>the return value of 'true' would to me intuitively mean "reading was successful".</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Trying to come up with a nicer API it seems to me that it's not worth cracking our head on for those mere two cases below, especially since there is still more return value checking in 657: 'if (ret < needed)' -- it makes it harder to read what is actually going on.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I appreciate the code dup removal, but I believe in this instance it comes at a cost, which is too high (both readability wise and time spent wise).</p><p style="white-space: pre-wrap; word-wrap: break-word;">(some more comments follow from before I reached that conclusion)</p><p>Patch set 4:<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/11911">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/11911/4//COMMIT_MSG">Commit Message:</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/11911/4//COMMIT_MSG@10">Patch Set #4, Line 10:</a> <code style="font-family:monospace,monospace">we do it in oter places.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">'h' key stuck? :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11911/4/src/gsm/ipa.c">File src/gsm/ipa.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/11911/4/src/gsm/ipa.c@548">Patch Set #4, Line 548:</a> <code style="font-family:monospace,monospace"> * \param[in] needed Home many bytes we have to read.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">'How'?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually, please rather just use the recv() arg name and refer there, as it is passed transparently:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">   size_t len   len parameter passed to recv()</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11911/4/src/gsm/ipa.c@553">Patch Set #4, Line 553:</a> <code style="font-family:monospace,monospace">       *ret = recv(fd, msg->tail, needed, 0);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This looks like it lacks some bounds checking: make sure that msgb has enough room for 'needed' at its tail? (Or do all callers cover that already?)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Would it make sense to derive the 'needed' (len) parameter from the msgb? (I guess not)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11911">change 11911</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/11911"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ic0147bffaf04b0baf97e5cca22948bd0e116668f </div>
<div style="display:none"> Gerrit-Change-Number: 11911 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 03 Dec 2018 18:10:47 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>