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 8: Code-Review-1 (16 comments) https://gerrit.osmocom.org/#/c/641/8//COMMIT_MSG Commit Message: Line 7: Adding SNDCP-XID encoder / decoder "Add SNDCP-XID encoder / decoder and unit test" https://gerrit.osmocom.org/#/c/641/8/openbsc/src/gprs/gprs_sndcp_xid.c File openbsc/src/gprs/gprs_sndcp_xid.c: Line 1635: return NULL; missing free of comp_fields in case of error Line 1641: return NULL; Though this also looks like a missing free of comp_fields in case of error, it actually isn't, because gprs_sndcp_decode_xid() does the talloc_free on comp_fields. While this is technically correct, I would prefer if you would always free comp_fields in this function instead of delegating to each and every error case of called functions. So, let the other return error, and here have e.g.: gprs_sndcp_parse_xid() { rc = gprs_sndcp_foo(...); if (rc) goto error_free_comp_fields rc = gprs_sndcp_bar(...); if (rc) goto error_free_comp_fields [...] return comp_fields; error_free_comp_fields: talloc_free(comp_fields); return NULL; } If you grep for goto, you will find numerous instances of this kind of error handling ... and this is the *only* way we use goto :) https://www.xkcd.com/292/ Line 1646: return NULL; same Line 1652: return NULL; same Line 1660: { personally, I would just call talloc_free(comp_fields) and drop this function. The callers generally can expect comp_fields to never be null. Line 1868: LOGP(DSNDCP, logl, "\n"); really log a blank line? :) https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/sgsn/Makefile.am File openbsc/tests/sgsn/Makefile.am: Line 34: $(top_builddir)/src/gprs/gprs_sndcp_xid.o \ sgsn-test is not using this, no need to add anything Line 42: -lgtp -lrt -lm this is not needed either https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/testsuite.at File openbsc/tests/testsuite.at: Line 136: AT_CHECK([$abs_top_builddir/tests/xid_sndcp/sndcp_xid_test], [], [expout], [ignore]) ah, so you decided against checking stderr. Line 138: rather not add a blank line in the end https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/xid_sndcp/Makefile.am File openbsc/tests/xid_sndcp/Makefile.am: Line 20: I know it's nitpicking, but generally we're pretty tight on whitespace in osmocom code ... drop the blank lines at EOF. https://gerrit.osmocom.org/#/c/641/8/openbsc/tests/xid_sndcp/sndcp_xid_test.c File openbsc/tests/xid_sndcp/sndcp_xid_test.c: Line 54: gprs_sndcp_dump_comp_fields(comp_fields, DSNDCP); would be nice to check this dump in experr? Line 60: printf("Rencoded: %s\n", osmo_hexdump_nospc(xid_r, rc)); "Reencoded" / "Re-encoded"? Line 240: gprs_sndcp_dump_comp_fields(comp_fields_dec, DSNDCP); again check this in experr? Line 276: } you need to add a stub for bssgp_prim_cb like in sgsn_test.c: /* stubs */ struct osmo_prim_hdr; int bssgp_prim_cb(struct osmo_prim_hdr *oph, void *ctx) { abort(); } -- To view, visit https://gerrit.osmocom.org/641 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2d63fe2550864cafef3156b1dc0629037c49c1e Gerrit-PatchSet: 8 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