Change in ...osmo-sgsn[master]: Split enum gprs_pmm_state into Iu and Gb counterparts

fixeria gerrit-no-reply at lists.osmocom.org
Thu Aug 29 23:08:08 UTC 2019


fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/15337 )

Change subject: Split enum gprs_pmm_state into Iu and Gb counterparts
......................................................................


Patch Set 1: Code-Review-1

(8 comments)

I like this change, but CR-1 because of two potential copy-paste mistakes.

https://gerrit.osmocom.org/#/c/15337/1/include/osmocom/sgsn/gprs_sgsn.h 
File include/osmocom/sgsn/gprs_sgsn.h:

https://gerrit.osmocom.org/#/c/15337/1/include/osmocom/sgsn/gprs_sgsn.h@43 
PS1, Line 43: PMM_
Any reason why it needs to be prefixed with 'PMM_', and not with 'MM_'? What is the meaning of 'P' here? Maybe we should rather use 'MM_GB_ST_' and 'MM_IU_ST_' prefixes for both enums? This naming would help to avoid confusion.


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c 
File src/gprs/gprs_gmm.c:

https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c@113 
PS1, Line 113: MM_IDLE
Looks like a copy-paste error to me, as 'gprs_mm_state_iu' enum defines different states (than 'gprs_mm_state_gb').


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c@181 
PS1, Line 181: mmctx_set_pmm_state
Same problem with naming, maybe we should also use '_iu_' / '_gb_' prefixes here?


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c@199 
PS1, Line 199: case PMM_DETACHED:
This change looks unrelated, but in general I am not against it.


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c@206 
PS1, Line 206: mmctx_set_mm_state
Same.


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_gmm.c@215 
PS1, Line 215: gprs_mm_state_iu_names
Looks like a mistake to me.


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_sgsn.c 
File src/gprs/gprs_sgsn.c:

https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_sgsn.c@230 
PS1, Line 230: 	ctx->gb.mm_state = MM_IDLE;
Please keep the same order, it was after the 'ctx->gmm_state' assignment.


https://gerrit.osmocom.org/#/c/15337/1/src/gprs/gprs_sgsn.c@265 
PS1, Line 265: 	ctx->iu.mm_state = PMM_DETACHED;
Same.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-sgsn/+/15337
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-sgsn
Gerrit-Branch: master
Gerrit-Change-Id: I6100d607da316da0595886c6968704dd9ccfbde9
Gerrit-Change-Number: 15337
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Comment-Date: Thu, 29 Aug 2019 23:08:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190829/d2f88727/attachment.html>


More information about the gerrit-log mailing list