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