libosmocore[master]: support for more cell ID list types in libosmocore

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.org
Tue Feb 27 19:49:47 UTC 2018


Patch Set 9: Code-Review-1

(16 comments)

erm, sorry to bombard with another burst of comments. Many are purely cosmetic, a few notable ones in-between. I believe you can tell the difference and make your choices...

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 the comment?


Line 47: 	} id_list;
why not just {...}id_list[MAXLEN] once? the size will be the same and it's less code repetition.

also thinking, since we have ARRAY_SIZE(), we could just define id_list[127] without a separate #define.


Line 73: /* deprecated */
we have an OSMO_DEPRECATED() macro to mark with a compiler warning. The only reason I am sometimes careful with those is that some of our builds now fail on any warnings. Actually we might want to make deprecation warnings non-fatal, so that we don't refrain from marking deprecation properly (like I'm doing lately, unfortunately).


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 associate it with the #define instead of the struct. (I'm not sure whether the OSMO_DEPRECATED() also works with structs, so far used it only with functions)


https://gerrit.osmocom.org/#/c/6509/9/src/gsm/gsm0808.c
File src/gsm/gsm0808.c:

Line 509: 	/* Mandatory elements! */
rofl emelents ... technically an unrelated cosmetic


Line 553:  * Create BSSMAP PAGING message
make sure to put full-stops at the end of sentences for doxygen parsing. Often it might not be strictly necessary, but rather do it always. (to indicate end-of-brief, end-of-param-doc etc)


Line 565: 	/* Mandatory emelents! */
rofl squared, seriously? :D


Line 571: 	memset(&cil2, 0, sizeof(cil2));
since holger pointed me at it, I like to use constructs like

  cil2 = (struct gsm0808_cell_id_list2){};

Above in the declaration, you can do it even without naming the struct again:

  struct gsm0808_cell_id_list2 cil2 = {};

This guarantees to init all members to their "constructor" value, in practice always zero.


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 validate that id_list_len has a proper value to avoid copying past the array end. Well, probably want that too somewhere, not necessarily needs to be here though.

Also you should probably add somewhere:

  osmo_static_assert(some_name, ARRAY_SIZE(cil2.id_list) >= ARRAY_SIZE(cil.id_list));

(sprinkle syntactic elements in the fashion of '((struct xxx_list2*)0)->' as needed, see other static assertions).

(osmo_static_assert can also live outside of function scopes.)


https://gerrit.osmocom.org/#/c/6509/9/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:

Line 595: 		for (i = 0; i < cil->id_list_len; i++) {
we could have a single for loop outside the switch ... ok granted, it's less code but runs the switch for each iteration. ok then forget it, micro optimisation vs code cosmetics, doesn't matter.


Line 600: 			msgb_put_u16(msg, id->cell_identity);
the parsing function puts each single parsing section in its separate function. The encoding function here contains all the encoding directly in the switch(). Does it makes sense to also put the encoding of single elements separately? Or, to also put the parsing all in the switch below?


Line 632: 		OSMO_ASSERT(false);
will an incoming message with an unsupported list type be able to crash the program? Maybe rather return -ENOTSUP? hmm, but this function is defined to not return errors ever.


Line 689: 	return gsm48_decode_lai(&lai, mcc, mnc, lac) != 0 ? -1 : 0;
(i'd drop the '!= 0', to me it makes it look like it is intended to reverse the logic but is just the same as without it)


Line 855: 	/* One byte for the cell ID descriminator + any remaining bytes in
"discriminator"


https://gerrit.osmocom.org/#/c/6509/9/tests/gsm0808/gsm0808_test.c
File tests/gsm0808/gsm0808_test.c:

Line 816: 	memset(&enc_cil, 0, sizeof(enc_cil));
just put ' = {};' above. Ah it's old code, doesn't matter which way then.


Line 817: 	enc_cil.id_discr = CELL_IDENT_BSS;
ha! this a bugfix?


-- 
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



More information about the gerrit-log mailing list