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/.
Stefan Sperling gerrit-no-reply at lists.osmocom.org
Patch Set 9:
(11 comments)
https://gerrit.osmocom.org/#/c/6509/9/include/osmocom/gsm/gsm0808_utils.h
File include/osmocom/gsm/gsm0808_utils.h:
Line 32: #define GSM0808_CELL_ID_LIST2_MAXLEN 128
> from (255-1)/2 the theoretical max is 127, would be nice to explain that in
Yes, comment improved in next patch set.
Line 47: } id_list;
> why not just {...}id_list[MAXLEN] once? the size will be the same and it's
Unfortunately, your ARRAY_SIZE() idea complicates the parse_cell_id_* functions in gsm0808_utils.c. At present these won't get to see the entire id_list, instead they see a type-specific pointer to a union member like &id_list.id_list_global[0]. So we'd have to either typedef the union in order to pass the entire union array to each such function, or pass a size...
I think I prefer either leaving it the way things are now, or using id_list[] as an array but leaving CELL_ID_LIST2_MAXLEN in place so these functions can use it. See next patch set.
Line 73: /* deprecated */
> we have an OSMO_DEPRECATED() macro to mark with a compiler warning. The onl
I'll leave this as it is for now.
https://gerrit.osmocom.org/#/c/6509/9/include/osmocom/gsm/protocol/gsm_08_08.h
File include/osmocom/gsm/protocol/gsm_08_08.h:
Line 505: * DEPRECATED: This definition of the cell identifier list is
> maybe rather move this down to the struct, I think doxygen will otherwise a
I'll move the #define above the comment.
https://gerrit.osmocom.org/#/c/6509/9/src/gsm/gsm0808.c
File src/gsm/gsm0808.c:
Line 553: * Create BSSMAP PAGING message
> make sure to put full-stops at the end of sentences for doxygen parsing. Of
Sure. Fixed in next patch set.
Line 565: /* Mandatory emelents! */
> rofl squared, seriously? :D
I have to admit I left this one there on purpose cause I liked the typo so much <3
Line 571: memset(&cil2, 0, sizeof(cil2));
> since holger pointed me at it, I like to use constructs like
Sure, I don't mind either way. Tweaked in next patch set.
Line 573: memcpy(cil2.id_list.id_list_lac, cil->id_list_lac, cil->id_list_len * sizeof(cil2.id_list.id_list_lac[0]));
> why not just copy the whole array, used or not. Otherwise you have to valid
The length check on line 568 above is not sufficient?
https://gerrit.osmocom.org/#/c/6509/9/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:
Line 600: msgb_put_u16(msg, id->cell_identity);
> the parsing function puts each single parsing section in its separate funct
The encoding case is much simpler then decoding. It doesn't need paranoid input verification and requires less local variables. So I think additional functions are not warranted.
Line 632: OSMO_ASSERT(false);
> will an incoming message with an unsupported list type be able to crash the
If this function is asked to encode something we don't actually support, we should assert and fix the bug in upper layers. This function is dealing with output, not input.
https://gerrit.osmocom.org/#/c/6509/9/tests/gsm0808/gsm0808_test.c
File tests/gsm0808/gsm0808_test.c:
Line 817: enc_cil.id_discr = CELL_IDENT_BSS;
> ha! this a bugfix?
Yes.
--
To view, visit https://gerrit.osmocom.org/6509
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7e754f538df0c83298a3c958b4e15a32fcb8abb
Gerrit-PatchSet: 9
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-HasComments: Yes