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

Stefan Sperling gerrit-no-reply at lists.osmocom.org
Tue Feb 27 15:15:24 UTC 2018


Patch Set 6:

(8 comments)

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))
> I see now, you're limiting by the maximum possible number that fits in a si
As explained above, the idea is to avoid growing beyond the old structure's size.


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
This grew out of an attempt to keep things compatible at least on the ABI level. Binaries compiled against old libosmocore will keep working against new libosmocore because sizes don't change and the LAC which old binaries care about is still in the same spot.


Line 69: struct gsm0808_cell_id_list {
> it is actually defined in 3GPP TS 48.008 3.2.2.27 Cell Identifier List ... 
Yes, as mentioned in the log message this changeset is currently breaking one known API consumer. See https://gerrit.osmocom.org/#/c/6518/


Line 70: 	uint8_t id_discr;
> We have enum CELL_IDENT in libosmocore/include/osmocom/gsm/protocol/gsm_08_
The values from this enum are in fact what this field has always been using. If we change the datatype to an enum then code compiled against old libosmocore will use the wrong offsets. The uint16_t was inherited from old code. It should really have been an enum in the first place. I'd rather change that in a second step unless we decide that keeping old binaries working is not important.


Line 71: 	union {
> I also thought of this as a union first, but since most of these elements a
I don't know. It seems to me that this is a perfect use case  for a union, precisely because it prevents ambiguity about which fields are valid. After all, a struct where only some fields are valid is pretty much like a union but with a risk of ambiguity and wasted space, isn't it?

I looked at existing structs and reused them where I could. But not all of them fit what's needed here. It seems that historically most of the osmo code cared about LAC only, and that's why these changes are needed now as we want to extend beyond that.

What I don't really like is that this union strongly mirrors the on-wire format so a unified representation might be better. Then again, a lot of code down the line cares about the distinction, e.g. the paging code wants to know which varianet of IDs appeared on the wire.


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 differen
The old API is defined in the wrong place (host-side struct in protocol-side header file), is a weird mix of wire-format sizes for data types which actually contain host-side representation of the data in host byte-order, and is insufficient for what it is supposed to represent because it cannot even encode all cell list types. In short, the old API is entirely broken and confused.
I discussed it with pmaier who added this old API, and he agreed that it was a mistake. 

We could keep it around for backwards compat if you prefer that, which also means keeping the old code that used it.
But since there is only one known user, I'd argue we can remove it safely and should. To ease this transition I designed the new API to remain ABI compatible with the old one, but I am not in love with that idea either and would be happy to break that, too.

If any third party is using this problematic API they must switch to the new one anyway when they upgrade libosmocore. I see no value in keeping their old code working in this situation.


Line 656: 	return gsm48_decode_lai(&lai, mcc, mnc, lac) != 0 ? -1 : 0;
> (I'd scrap the '!= 0')
I tend to use != 0 if there's a negative error return or zero in case of success. and I don't use !=0 if the return value is really a boolean. But that's a minor detail, I'm happy with either style.

In fact, gsm48_decode_lai() never fails and should probably return void.


Line 829: 	return 1 + (int)bytes_elem;
> maybe comment that it is the id_discr + N elements ... I was a bit confused
Yes, added in next patch set.


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



More information about the gerrit-log mailing list