<p style="white-space: pre-wrap; word-wrap: break-word;">I would have liked to just accept this, alas, there are some hidden issues in there...</p><p><a href="https://gerrit.osmocom.org/11656">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_comp.c">File src/gprs/gprs_sndcp_comp.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/11656/1/src/gprs/gprs_sndcp_comp.c@99">Patch Set #1, Line 99:</a> <code style="font-family:monospace,monospace">     OSMO_ASSERT(comp_entity->compclass != SNDCP_XID_INVALID_COMPRESSION);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(later on I refer to this "-1")</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c">File src/gprs/gprs_sndcp_xid.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/11656/1/src/gprs/gprs_sndcp_xid.c@456">Patch Set #1, Line 456:</a> <code style="font-family:monospace,monospace">              OSMO_ASSERT(comp_field->algo.dcomp >= 0 || comp_field->algo.dcomp <= 0x1f);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">wait a second, the original condition is always true, it should be &&, not ||. Maybe a separate patch should first fix that condition, and then this patch can apply the enum change without functional changes.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@460">Patch Set #1, Line 460:</a> <code style="font-family:monospace,monospace">          break; /* not reached */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(I think I usually just omit that break, seems to work)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@538">Patch Set #1, Line 538:</a> <code style="font-family:monospace,monospace">          return SNDCP_XID_INVALID_COMPRESSION;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">before, there is a comparison " != -1" replaced with " != SNDCP_XID_INVALID_COMPRESSION". Now here I notice that -EINVAL isn't actually -1. So is this also fixing a problem? (preferably applying the enums has absolutely no functional change, and we'd fix the problem in a separate patch, presumably before this one)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1299">Patch Set #1, Line 1299:</a> <code style="font-family:monospace,monospace">           return SNDCP_XID_INVALID_COMPRESSION;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">not sure whether this makes sense. Above and below, -EINVAL is returned or a length when successful.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1444">Patch Set #1, Line 1444:</a> <code style="font-family:monospace,monospace">              if (compclass != SNDCP_XID_INVALID_COMPRESSION) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It's a bit awkward to follow this logic, both before and after this patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Before, if compclass < 0, we would return -EINVAL, now we continue the loop?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The causality seems different now, we don't check the comp_field.algo anymore. I'm not sure whether that makes a practical difference, but for the enum patch it would be nicer to not vaguely suspect functional changes. (Are there list entries that should be skipped, or is this plain sanity checking that would be fine either way around? could we also shed all this checking because it is never going to be a problem anyway?)</p><p style="white-space: pre-wrap; word-wrap: break-word;">Reading and complexity wise, it would be nicer if there were just one switch() on top, not nested inside if on the same compclass value?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1474">Patch Set #1, Line 1474:</a> <code style="font-family:monospace,monospace">       compclass = gprs_sndcp_get_compression_class(comp_field_dst);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what is the relation between gprs_sndcp_get_compression_class() and the gprs_sndcp_comp.compclass enum value? Do we have to invoke the nontrivial if cascade inside gprs_sndcp_get_compression_class() all the time? I would kind of assume the compclass member gets assigned once during init, and from there on it can be used directly to decide which algo.* union member applies, without re-evaluating the comp_field->rfc*params items?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or wait, I think I'm mixing up two structs. Anyway, do you see what I mean?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1878">Patch Set #1, Line 1878:</a> <code style="font-family:monospace,monospace">                      LOGP(DSNDCP, logl, "   algo=%d;\n", comp_field->algo.dcomp);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">heh</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11656">change 11656</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/11656"/><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: I3771a5c59f4e6fee24083b3c914965baf192cbd7 </div>
<div style="display:none"> Gerrit-Change-Number: 11656 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Stefan Sperling <ssperling@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-CC: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 08 Nov 2018 15:54:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>