Change in osmo-sgsn[master]: use enums consistently instead of falling back to int

Neels Hofmeyr gerrit-no-reply at
Thu Nov 8 15:54:19 UTC 2018

Neels Hofmeyr has posted comments on this change. ( )

Change subject: use enums consistently instead of falling back to int

Patch Set 1:


I would have liked to just accept this, alas, there are some hidden issues in there...
File src/gprs/gprs_sndcp_comp.c:
PS1, Line 99: 	OSMO_ASSERT(comp_entity->compclass != SNDCP_XID_INVALID_COMPRESSION);
(later on I refer to this "-1")
File src/gprs/gprs_sndcp_xid.c:
PS1, Line 456: 		OSMO_ASSERT(comp_field->algo.dcomp >= 0 || comp_field->algo.dcomp <= 0x1f);
wait a second, the original condition is always true, it should be &&, not ||. Maybe a separate patch should first fix that condition, and then this patch can apply the enum change without functional changes.
PS1, Line 460: 		break; /* not reached */
(I think I usually just omit that break, seems to work)
before, there is a comparison " != -1" replaced with " != SNDCP_XID_INVALID_COMPRESSION". Now here I notice that -EINVAL isn't actually -1. So is this also fixing a problem? (preferably applying the enums has absolutely no functional change, and we'd fix the problem in a separate patch, presumably before this one)
not sure whether this makes sense. Above and below, -EINVAL is returned or a length when successful.
PS1, Line 1444: 		if (compclass != SNDCP_XID_INVALID_COMPRESSION) {
It's a bit awkward to follow this logic, both before and after this patch.

Before, if compclass < 0, we would return -EINVAL, now we continue the loop?

The causality seems different now, we don't check the comp_field.algo anymore. I'm not sure whether that makes a practical difference, but for the enum patch it would be nicer to not vaguely suspect functional changes. (Are there list entries that should be skipped, or is this plain sanity checking that would be fine either way around? could we also shed all this checking because it is never going to be a problem anyway?)

Reading and complexity wise, it would be nicer if there were just one switch() on top, not nested inside if on the same compclass value?
PS1, Line 1474: 	compclass = gprs_sndcp_get_compression_class(comp_field_dst);
what is the relation between gprs_sndcp_get_compression_class() and the gprs_sndcp_comp.compclass enum value? Do we have to invoke the nontrivial if cascade inside gprs_sndcp_get_compression_class() all the time? I would kind of assume the compclass member gets assigned once during init, and from there on it can be used directly to decide which algo.* union member applies, without re-evaluating the comp_field->rfc*params items?

Or wait, I think I'm mixing up two structs. Anyway, do you see what I mean?
PS1, Line 1878: 			LOGP(DSNDCP, logl, "   algo=%d;\n", comp_field->algo.dcomp);

To view, visit
To unsubscribe, or for help writing mail filters, visit

Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3771a5c59f4e6fee24083b3c914965baf192cbd7
Gerrit-Change-Number: 11656
Gerrit-PatchSet: 1
Gerrit-Owner: Stefan Sperling <ssperling at>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-CC: Neels Hofmeyr <nhofmeyr at>
Gerrit-Comment-Date: Thu, 08 Nov 2018 15:54:19 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <>

More information about the gerrit-log mailing list