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