Mykola Shchetinin has posted comments on this change. ( https://gerrit.osmocom.org/13744 )
Change subject: gprs_gmm: send Service Reject when no PDP ctxs are available. ...................................................................... Patch Set 4: (5 comments) https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c File src/gprs/gprs_gmm.c: https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1620 PS2, Line 1620: bool all_ms_ctx_present_on_sgsn(struct sgsn_mm_ctx *mmctx, > You probably want to add spec chapter and section in this comment too. Not sure about this place. The function does only perform a check. Well, do you mean the same section which mentioned in the change below? About "Service request procedure not accepted by the network". Yeah, that makes sense, as the function seems to be only related to that "part" of behavior https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1625 PS2, Line 1625: size_t i; > where does this 16 come from? Can we please have a define (if we don't have > already) for it? Done https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1628 PS2, Line 1628: if (!(pdp_status[pdp_nsapi / 8] & (1 << (pdp_nsapi - 8 * i)))) > While fine at runtime, you are actually assigning an integer value (bitmask) > to a bool here, that's […] Done https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1630 PS2, Line 1630: > Can we actually have something like: […] Done https://gerrit.osmocom.org/#/c/13744/2/src/gprs/gprs_gmm.c@1920 PS2, Line 1920: if (TLVP_PRESENT(&tp, GSM48_IE_GMM_PDP_CTX_STATUS)) { > You probably need to drop this comment too right? Hm, it seems fair to leave it as it correctly describes what is happening inside the condition. And it doesn't add redundancy to my comment below, does it? -- To view, visit https://gerrit.osmocom.org/13744 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: If610cbef17c25ec44e65d4f1b2340d102c560437 Gerrit-Change-Number: 13744 Gerrit-PatchSet: 4 Gerrit-Owner: Mykola Shchetinin <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Mykola Shchetinin <[email protected]> Gerrit-Reviewer: lynxis lazus <[email protected]> Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Wed, 24 Apr 2019 11:34:27 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
