openbsc[master]: Added code to control GPRS TCP/IP header compression.

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/.

Harald Welte gerrit-no-reply at lists.osmocom.org
Thu Aug 4 15:00:27 UTC 2016


Patch Set 7: Code-Review-1

(11 comments)

https://gerrit.osmocom.org/#/c/642/7/openbsc/include/openbsc/gprs_llc.h
File openbsc/include/openbsc/gprs_llc.h:

PS7, Line 150: s
if struct comp_ent is never used outside of gprs_llc_llme (Not sure if it is), then it might make sense to have it as anonymouse structure as part of gprs_llc_llme, i.e.

struct gprs_llc_llme {
...
        struct {
                struct llist_head proto;
                struct llist_head data;
        } comp;


Line 182: 	/* Copy of the XID fields we sent */
which XID fields?  Of the latest XID exchange?


https://gerrit.osmocom.org/#/c/642/7/openbsc/include/openbsc/gprs_sndcp_hdrcomp.h
File openbsc/include/openbsc/gprs_sndcp_hdrcomp.h:

Line 34: struct gprs_sndcp_hdrcomp_compression_entity {
hdrcom_comrpression is long and redundant.  I would prefer sndcp_hdrcomp_entity, but as you use the gprs_ prefix everywhere, you might keep that at the beginning (or remove it everywhere, like I suggested at some other place).


https://gerrit.osmocom.org/#/c/642/7/openbsc/src/gprs/gprs_sndcp_comp_entity.c
File openbsc/src/gprs/gprs_sndcp_comp_entity.c:

Line 121: 	struct llist_head *lh, *lh2;
not mandatory, but we typically call the list_head variables not after what they are, but after what they point to.  So *ce for 'compression entity' might be more commonly seen in the code.  But this is JFYI.


Line 258: 	if (comp_entities) {
please remove this kind of check, _unless_ there is a valid use case in the real world, where this function encounters a NULL argument as comp_entities.

Also, normally the check would be inverse, i.e.

'if (!comp_entities) ... bail out' which makes sure the regular code path has one level less of indent

Finally, if those checks only exist to catch programming errors, the general approach is OSMO_ASSERT(comp_entities) which would crash with a backtrace in case it ever was NULL.


https://gerrit.osmocom.org/#/c/642/7/openbsc/src/gprs/gprs_sndcp_hdrcomp.c
File openbsc/src/gprs/gprs_sndcp_hdrcomp.c:

Line 155: 		//      packet[0] &= 0x7F;      
extra whitespace at end of line


PS7, Line 155: //
avoid C++ style comments unless they are to be removed when implementation is ready to be merged.


Line 162: 		packet[0] &= 0x4F;	
extra whitespace at end of line


Line 240: 	    gprs_sndcp_comp_entity_find_by_comp(comp_entities, pcomp);
too long function names for my taste.

sndcp_c_ent_by_comp() should be sifficient? Not mandatory, but I just think the code is much more readable if the function name dosen't occupy half of the line.


Line 316: 	    gprs_sndcp_comp_entity_find_comp_by_comp_index(comp_entity,
sndcp_c_ent_by_c_index() ?


Line 340: static uint16_t header_checksum(uint8_t * iph, unsigned int ihl)
test code should be part of the autotest test suite in /tests/


-- 
To view, visit https://gerrit.osmocom.org/642
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia00260dc09978844c2865957b4d43000b78b5e43
Gerrit-PatchSet: 7
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list