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 16: Code-Review-1
(9 comments)
https://gerrit.osmocom.org/#/c/642/16/openbsc/include/openbsc/gprs_sndcp_comp.h
File openbsc/include/openbsc/gprs_sndcp_comp.h:
Line 39: int nsapi[11]; /* Applicable NSAPIs (default 0) */
it might be better to introduce #defines with meaningful names rather than magic numbers here, but it is not super-critical, I think. Also, why use a signed integer, if the actual value is uint8_t (or maybe uint16_t? I don't remember) but certainly not a signed integer?
Line 43: int comp[16]; /* see also: 6.5.1.1.5 and 6.6.1.1.5 */
why use a signed integer, if the value is (I believe) uint8_t?
Line 67: *comp_entities, int entity);
comment regarding 'int' type applies to all function prototypes, too.
https://gerrit.osmocom.org/#/c/642/16/openbsc/src/gprs/gprs_sndcp_comp.c
File openbsc/src/gprs/gprs_sndcp_comp.c:
Line 53: comp_field->comp_len * sizeof(int));
sizeof(comp_entity->comp)?
Line 59: comp_entity->nsapi_len * sizeof(int));
same as above. never use the base type in length calculation, always use the name of the struct member. This way a change in struct/type definition doesn't break all over the code in subtle ways.
Line 153: llist_for_each_safe(ce, ce2, comp_entities) {
hierarchical talloc shouldn't need that, unless there is a way how the entries in the list could not be siblings of the entity (in which case this should probably be documented here).
Line 213: if (comp_entity) {
we always return early in case of error, rather than having if statements for the successful case.
----------
if (!comp_entity)
return NULL
success case here...
----------
https://gerrit.osmocom.org/#/c/642/16/openbsc/src/gprs/sgsn_vty.c
File openbsc/src/gprs/sgsn_vty.c:
Line 1086: NO_STR "compression\n"
the no-string has "compression" as help for the "compression" node, while the command below has "Configure compression". Best to introduce a COMPRESSION_STR and use it from both places in order to achieve consistency.
Line 1098: "number\n")
this should probably be named "Number of compression state slots", too. The fact that 1-256 is a number is probably clear already :)
--
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: 16
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-HasComments: Yes