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

[Neels Hofmeyr]

Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11656 )

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


Patch Set 1:

(8 comments)

I would have liked to just accept this, alas, there are some hidden issues in there...

https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_comp.c
File src/gprs/gprs_sndcp_comp.c:

https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_comp.c@99
PS1, Line 99: 	OSMO_ASSERT(comp_entity->compclass != SNDCP_XID_INVALID_COMPRESSION);
(later on I refer to this "-1")


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c
File src/gprs/gprs_sndcp_xid.c:

https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@456
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.


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@460
PS1, Line 460: 		break; /* not reached */
(I think I usually just omit that break, seems to work)


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@538
PS1, Line 538: 		return SNDCP_XID_INVALID_COMPRESSION;
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)


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1299
PS1, Line 1299: 		return SNDCP_XID_INVALID_COMPRESSION;
not sure whether this makes sense. Above and below, -EINVAL is returned or a length when successful.


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1444
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?


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1474
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?


https://gerrit.osmocom.org/#/c/11656/1/src/gprs/gprs_sndcp_xid.c@1878
PS1, Line 1878: 			LOGP(DSNDCP, logl, "   algo=%d;\n", comp_field->algo.dcomp);
heh



-- 
To view, visit https://gerrit.osmocom.org/11656
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

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 sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-CC: Neels Hofmeyr <nhofmeyr at sysmocom.de>
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: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181108/bc98c3f5/attachment.htm>