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 2: Code-Review-1
(16 comments)
https://gerrit.osmocom.org/#/c/638/2/openbsc/include/openbsc/gprs_llc_xid.h
File openbsc/include/openbsc/gprs_llc_xid.h:
Line 40: int gprs_llc_compile_xid(struct llist_head *xid_fields, uint8_t * bytes,
the input parameter could deserver a 'const'
PS2, Line 44: int gprs_llc_parse_xid(struct llist_head *xid_fields
instead of having the caller pass in an already-allocated llist_head, why not make the function return the 'llist_head *'? In case of error, NULL can be returned.
PS2, Line 44: uint8_t * bytes
the input parameter should use 'const uint8_t *'
https://gerrit.osmocom.org/#/c/638/2/openbsc/src/gprs/gprs_llc_xid.c
File openbsc/src/gprs/gprs_llc_xid.c:
PS2, Line 41: static int
: decode_xid_field(uint8_t *bytes, uint8_t bytes_len,
: struct gprs_llc_xid_field *xid_field)
why not allocate the xid_field (including the required xid_field->data) inside this function and returning a 'struct gprs_llc_xid_field'? It seems a bit odd that the caller has to allocate the structure, but not the data.
also, input parameter should be 'const'
PS2, Line 83: xid_field->data =
: talloc_zero_size(NULL, xid_field->data_len);
whenever we talloc() something, we should pass in a meaningful context. if xid_field is itself talloc'ed, we should use xid_field as context. If we don't know any context, the function's first argument should be a 'void *ctx' that is then used as context.
PS2, Line 85: memcpy
you can use talloc_memdup() as a short hand for allocation + memcpy
PS2, Line 97: struct gprs_llc_xid_field *xid_field
input parameter should be const
Line 150: memset(bytes, 0, bytes_maxlen);
I would remove that. we are filling the information in two lines below anyway.
PS2, Line 156: /* Immediately stop on error */
commenting is nice, but a "if (rc < 0) return rc" doesn't need one, it's obvious :)
PS2, Line 174:
extra space; const missing
PS2, Line 180: while (1) {
endless loops must have a strong exist condition. Are there cases in which the return value of decode_xid_field could be zero? In that case we would loop forever?
Line 209: llist_for_each_entry(xid_field, xid_fields, list) {
if 'gprs_llc_parse_xid' would itself allocate the initial llist_head and return it (and allocate all xid_fields from it), you could simply replace this entire function with a single call to talloc_free() and let the hierarchical allocator do its work.
PS2, Line 227: NULL
have caller pass in a context
PS2, Line 231: NULL
use duplicate_of_xid_field as context
Line 236: memset(&duplicate_of_xid_field->list, 0,
don't do that, but rather use one of the linuxlist.h macros wich will use well-defined POISON values.
PS2, Line 249: LOGL_DEBUG
might make sense to have the caller specify the log level (we might want to print it as part of an error, not just debug.
--
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: 2
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