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

historical

Patch Set 6: Code-Review-1

(8 comments)

I'd go more towards API stability (deprecate old and add new API naming); and I'd go for less definitions. My opinions are of course not imperative, let's see what others think of my remarks...

https://gerrit.osmocom.org/#/c/6509/6/include/osmocom/gsm/gsm0808_utils.h
File include/osmocom/gsm/gsm0808_utils.h:

Line 64: #define CELL_ID_LIST_GLOBAL_MAXLEN	(CELL_ID_LIST_MAXLEN / sizeof(struct osmo_cell_global_id))
gosh, I'd just define like 128 items for all and be done with it

Line 69: struct gsm0808_cell_id_list {
it is actually defined in 3GPP TS 48.008 3.2.2.27 Cell Identifier List ... so I guess the gsm0808 was a misnomer, I wouldn't carry that through. (Or is the definition copied around several times?)

The new convention is to prefix libosmocore stuff with osmo_*, so might go for osmo_cell_identifier_list.

Then we might also keep the old gsm0808_cell_id_list and mark deprecated (maybe even just add "DEPRECATED" to the doc string and hint at the new api). Even if we were never using this anywhere, the API is out there and we don't want to make compiling against newer libosmocore harder than necessary? If we *are* using it out there, merging this would break builds and require immediate merging of other patches.

Line 70: 	uint8_t id_discr;
> I meant "neither an enum nor is there a comment" ;)
We have enum CELL_IDENT in libosmocore/include/osmocom/gsm/protocol/gsm_08_08.h with the exact coding needed here. It has a weird non-OSMO name, but it exists.

Line 71: 	union {
I also thought of this as a union first, but since most of these elements are heavily repeated, I'd actually simply have a osmo_cell_global_id and a uint16_t rac. Then depending on above id_discr, some of their elements remain unused. It would spare a number of struct definitions you can just omit above. Note that we already have a multitude of similar struct definitions, see gprs_ra_id and the whole bunch around osmo_cell_global_id, I don't think we really need more of those.

  #define CELL_ID_LIST_MAXLEN 128

  struct osmo_cell_id_list {
    enum CELL_IDENT type;
    unsigned int len;

    struct {
      struct osmo_cell_global_id cgi;
      uint16_t rac;
    } item[CELL_ID_LIST_MAXLEN];
  }

what do you think?

One could argue a drawback is that consumers need to be aware which items are populated and which are invalid, but we anyway don't have a 1:1 mapping between the enum CELL_IDENT and union members (some are "empty"), so need to be careful about that anyway.

https://gerrit.osmocom.org/#/c/6509/6/include/osmocom/gsm/protocol/gsm_08_08.h
File include/osmocom/gsm/protocol/gsm_08_08.h:

Line 506
(see above, I'd keep this and deprecate)

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

Line 576: uint8_t gsm0808_enc_cell_id_list(struct msgb *msg,
The signature of this function changes silently, by a dramatically different gsm0808_cell_id_list struct. I think I would go for a new function and deprecating this one.

Then again it seems to me I'm the most pedantic on these cosmetic issues; nevertheless the "compiling old code against new libosmocore" argument remains valid.

Line 656: 	return gsm48_decode_lai(&lai, mcc, mnc, lac) != 0 ? -1 : 0;
(I'd scrap the '!= 0')

Line 829: 	return 1 + (int)bytes_elem;
maybe comment that it is the id_discr + N elements ... I was a bit confused at first, like "what about the IE discriminator and len", then realized it is handled outside of this function.

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