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 8: Code-Review-1 (6 comments) https://gerrit.osmocom.org/#/c/638/8/openbsc/src/gprs/gprs_llc_xid.c File openbsc/src/gprs/gprs_llc_xid.c: Line 209: struct llist_head *gprs_llc_free_xid(struct llist_head *xid_fields) why does this function have a return value if it always returns NULL? It's customary for free() functions to be void. You can of course deviate from that, but it needs to have a good reason (+ probably explanation). Line 211: if (xid_fields != NULL) talloc_free(NULL), like free(NULL) are perfectly valid and well-defined operations, no need for any check Line 226: duplicate_of_xid_field = talloc_zero(ctx, struct gprs_llc_xid_field); no need for talloc_zero() with its zero-initialization, if you for sure overwrite all of the allocated memory in the next line using memcpy Line 228: sizeof(struct gprs_llc_xid_field)); please use sizeof(*duplicate_of_xid_field), as explained in other feedback comment before Line 230: talloc_zero_size(duplicate_of_xid_field, xid_field->data_len); same as above regarding 'zero'. also see talloc_memdup. Line 272: LOGP(DSNDCP, logl, this is a function that is called 'gprs_llc_...' but it uses DSNDCP for logging. Either use DLLC, or if it is sometimes either of both, pass the 'int log_subsys' into the function by the caller. -- To view, visit https://gerrit.osmocom.org/638 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia06e4cb08bf9b48c2a4682606d1b1a91d19a9d37 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: Max <msuraev at sysmocom.de> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-HasComments: Yes