Change in osmo-pcu[master]: RLC/MAC: implement decoding of [EGPRS] Packet Channel Request

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

fixeria gerrit-no-reply at lists.osmocom.org
Sat Apr 4 14:12:07 UTC 2020


fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/17718 )

Change subject: RLC/MAC: implement decoding of [EGPRS] Packet Channel Request
......................................................................


Patch Set 1:

(5 comments)

> Are you planning to submit similar to wireshark?

Yes, as soon as I find time for that...

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c 
File src/gsm_rlcmac.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@5337 
PS1, Line 5337:   M_CHOICE       (PacketChannelRequest_11b_t, Type,
> I think you need to first parse Type, or are you doing it somewhere else? See for instance: […]
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.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6319 
PS1, Line 6319:   struct bitvec bv = {
> If we have an API to allocate better use that, in order not to assume implementation details.
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.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6325 
PS1, Line 6325:   switch (req->type) {
> In decode functions afaik the data structure is memzeroes by default. […]
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.


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/src/gsm_rlcmac.c@6333 
PS1, Line 6333:         descr = CSNDESCR(EGPRS_PacketChannelRequest_t);
> Better have one function for each initial csnStreamInit like we do with others.
Why is it better? What would we benefit?


https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp 
File tests/rlcmac/RLCMACTest.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/17718/1/tests/rlcmac/RLCMACTest.cpp@481 
PS1, Line 481: 	static const uint16_t GPRSPktCh11bReqs[] = {
> Did you think about implementing encoding and using the struct and pass it to the encoder then to th […]
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.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/17718
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I96df3352856933c9140177b2801a2c71f4134183
Gerrit-Change-Number: 17718
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Sat, 04 Apr 2020 14:12:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200404/db983682/attachment.htm>


More information about the gerrit-log mailing list