<p><a href="https://gerrit.osmocom.org/13744">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c">File src/gprs/gprs_gmm.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/13744/2/src/gprs/gprs_gmm.c@1620">Patch Set #2, Line 1620:</a> <code style="font-family:monospace,monospace">bool all_ms_ctx_present_on_sgsn(struct sgsn_mm_ctx *mmctx,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You probably want to add spec chapter and section in this comment too.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure about this place. The function does only perform a check. Well, do you mean the same section which mentioned in the change below? About "Service request procedure not accepted by the network". Yeah, that makes sense, as the function seems to be only related to that "part" of behavior</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1625">Patch Set #2, Line 1625:</a> <code style="font-family:monospace,monospace">   size_t i;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">where does this 16 come from? Can we please have a define (if we don't have already) for it?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1628">Patch Set #2, Line 1628:</a> <code style="font-family:monospace,monospace">                if (!(pdp_status[pdp_nsapi / 8] & (1 << (pdp_nsapi - 8 * i))))</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">While fine at runtime, you are actually assigning an integer value (bitmask) to a bool here, that's  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1630">Patch Set #2, Line 1630:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Can we actually have something like: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1920">Patch Set #2, Line 1920:</a> <code style="font-family:monospace,monospace">  if (TLVP_PRESENT(&tp, GSM48_IE_GMM_PDP_CTX_STATUS)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You probably need to drop this comment too right?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Hm, it seems fair to leave it as it correctly describes what is happening inside the condition. And it doesn't add redundancy to my comment below, does it?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13744">change 13744</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/13744"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-sgsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: If610cbef17c25ec44e65d4f1b2340d102c560437 </div>
<div style="display:none"> Gerrit-Change-Number: 13744 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: Mykola Shchetinin <mykola@pentonet.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Mykola Shchetinin <mykola@pentonet.com> </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 24 Apr 2019 11:34:27 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>