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/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch Set 23: (25 comments) https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_sndcp_comp.c File openbsc/src/gprs/gprs_sndcp_comp.c: Line 40: const struct weird line breaks Line 90: OSMO_ASSERT(comp_entity->nsapi_len > 0) missing ';' and weird indent below Line 99: if (comp_entity->compclass == SNDCP_XID_PROTOCOL_COMPRESSION) { I was going to suggest the below, but I notice that data compression might be added some day... will it? if (comp_entity->compclass != SNDCP_XID_PROTOCOL_COMPRESSION) { [...] return NULL; } if (gprs_.... Line 122: } else { from above theck this is never reached, but again a placeholder for data compression... so ok Line 151: OSMO_ASSERT(comp_entities); quite a long comment on an obvious issue. BTW, if you were to comp_free the same entities twice by accident, this assert wouldn't hit: though talloc_free() freed the mem, the comp_entities pointer would still be != NULL. So usually, asserts like this don't really have any benefit unless you *expect* calling code paths to handle NULL pointers to indicate unset data or something. Line 163: comp_entity->entity); hmm, not implemented, dead code, but making believe that it does something useful? Line 177: OSMO_ASSERT(comp_entities); (one of those asserts) Line 181: comp_entity_to_delete = comp_entity; 'break;' when found? Line 195: comp_entity_to_delete->entity); (make-believe dead code again) Line 206: struct llist_head weird line breaks Line 215: OSMO_ASSERT(comp_field); (asserts... and more below) Line 232: struct gprs_sndcp_comp *gprs_sndcp_comp_by_comp(const struct llist_head line breaks... and more below Line 283: * all sort of compression and is assigned fix. So we what do you mean by "is assigned fix"? Line 291: return i + 1; whoa, why '+ 1'? I believe this will cause a lot of confusion. In C, we should have 0-indexing, only translating to 1-indexing for humans if really necessary, e.g. in the VTY. Now looking at the only caller gprs_sndcp_pcomp_expand() that feeds the returned value to gprs_sndcp_pcomp_rfc1144_expand(). I notice that this returns an int while gprs_sndcp_pcomp_rfc1144_expand() takes a uint8_t argument. Also in gprs_sndcp_pcomp_rfc1144_expand() the returned value seems to be expected to be exactly either 1 or 2. Could you please explain a bit further before we decide what to do? Or a different name like 'pcomp_type' could resolve the confusion; maybe with an enum defining those indexes explicitly? Line 318: return comp_entity->comp[comp_index - 1]; now -1 again :/ ;) https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_sndcp_pcomp.c File openbsc/src/gprs/gprs_sndcp_pcomp.c: Line 98: uint8_t *comp_ptr; /* Required by slhc_compress() */ kind of superfluous comment :) Line 141: break; this is where the 1-indexed pcomp_index value is used. What if pcomp_index < 1 or > 2? Would be good to add error messages (or asserts if it's never to be expected) in a 'default:' case. Line 150: /* Just in case the phone tags uncompressed tcp-datas ('data' is already the plural of 'datum') Line 179: OSMO_ASSERT(comp_entities); (asserts) Line 183: memcpy(data_o, data_i, len); since this happens twice, this could be a case for 'goto skip_decompression;' and having the memcpy only once below 'return rc;' at the bottom. Line 202: OSMO_ASSERT(comp_entity->algo == RFC_1144); should these two asserts rather be error messages? assertions terminate the program and disrupt service... Line 231: OSMO_ASSERT(comp_entities); (asserts) Line 249: OSMO_ASSERT(comp_entity->algo == RFC_1144); error messages? https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/sgsn_libgtp.c File openbsc/src/gprs/sgsn_libgtp.c: Line 330: return rc_pdp; 'return 0;' or just 'return sndcp_sn_xid_req(...)' above https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/sgsn_vty.c File openbsc/src/gprs/sgsn_vty.c: Line 274: g_cfg->pcomp_rfc1144.s01+1, VTY_NEWLINE); weird indenting -- 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: 23 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-HasComments: Yes