<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Are you planning to submit similar to wireshark?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, as soon as I find time for that...</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718">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/osmo-pcu/+/17718/1/src/gsm_rlcmac.c">File src/gsm_rlcmac.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/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@5337">Patch Set #1, Line 5337:</a> <code style="font-family:monospace,monospace">  M_CHOICE       (PacketChannelRequest_11b_t, Type,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think you need to first parse Type, or are you doing it somewhere else? See for instance: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually M_CHOICE does parsing for us, check out 'case CSN_CHOICE' in csnStreamDecoder(). The definition of IA_EGPRS_t looks wrong to me. None of the other definitions is using M_CHOICE like that.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6319">Patch Set #1, Line 6319:</a> <code style="font-family:monospace,monospace">  struct bitvec bv = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If we have an API to allocate better use that, in order not to assume implementation details.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Lifetime of this structure is limited by the execution scope of this function, so I don't think we really need to allocate it on heap just because we have an API for that. Moreover, this is not the only place where a bitvector is allocated on stack, there is at least one such allocation in libosmocore and one more in osmo-pcu.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6325">Patch Set #1, Line 6325:</a> <code style="font-family:monospace,monospace">  switch (req->type) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">In decode functions afaik the data structure is memzeroes by default. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">You mean other decoding functions in this file or csnStreamDecoder()? The later does not zero-initialize the structure, and that's nice because we can catch bugs with AddressSanitizer or Valgrind. As you probably know, I am against using zero-initialization without the real need for that.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6333">Patch Set #1, Line 6333:</a> <code style="font-family:monospace,monospace">        descr = CSNDESCR(EGPRS_PacketChannelRequest_t);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Better have one function for each initial csnStreamInit like we do with others.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is it better? What would we benefit?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp">File tests/rlcmac/RLCMACTest.cpp:</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/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp@481">Patch Set #1, Line 481:</a> <code style="font-family:monospace,monospace">     static const uint16_t GPRSPktCh11bReqs[] = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Did you think about implementing encoding and using the struct and pass it to the encoder then to th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since we're on the BTS/PCU side, we never need to encode RACH blocks on practice. So I think it's worthless to add encoding API just for a unit test. I am happy with decoding results.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/17718">change 17718</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-pcu/+/17718"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I96df3352856933c9140177b2801a2c71f4134183 </div>
<div style="display:none"> Gerrit-Change-Number: 17718 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: fixeria <axilirator@gmail.com> </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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Sat, 04 Apr 2020 14:12:07 +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"> Gerrit-MessageType: comment </div>