Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12860 )
Change subject: GSUP: add inter-MSC handover related msgs and IEs ...................................................................... Patch Set 2: (24 comments) In general, I would like to keep this patch unmerged before I have osmo-msc's inter-MSC HO pretty much complete and working. Likely more insights and needs about the protocol will arise from chiseling out the details. But it would be nice to continue the review process nevertheless; just not merge it yet (so we don't need to worry about api compat later). https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h File include/osmocom/gsm/gsup.h: https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@191 PS2, Line 191: OSMO_GSUP_MSGT_E_PROCESS_ACCESS_SIGNALLING_ERROR = 0b01000010, the Process Access Signalling and Forward Access Signalling will never return an Error. I know there was some obscure reason, but can't we get around having to define them? https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@196 PS2, Line 196: OSMO_GSUP_MSGT_E_CLOSE > As far as I can see, this comes from TCAP. […] actually, I did model an "abort" message in my osmo-msc/doc/interMSC_HO_GSUP_msgs.txt, and ... https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@197 PS2, Line 197: OSMO_GSUP_MSGT_E_ABORT = 0b01001011, ... and here is an Abort message. @vadim, what do you mean? https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@338 PS2, Line 338: an_apdu > I would use a pointer here. The osmo_gsup_message is already quite big... size is IMHO not a good reason. Is there another one? Otherwise, no need to extract two ints and a pointer. https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@341 PS2, Line 341: cause_sm > Does SM mean "Short Message"? If yes, we already have "sm_rp_cause". apparently means "Session Management" :( gsm48_gsm_cause in gsm_04_08_gprs.h says: "definition in 3GPP TS 24.008 10.5.6.6 / Table 10.5.157" I was going to call it cause_rr or something, but the spec does have this as a name. https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h File include/osmocom/gsm/gsup_handover.h: https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h@19 PS2, Line 19: uint8_t > const? agree https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h@20 PS2, Line 20: if this is all there is to this API, then let's just place these in the common gsup.[hc]? https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c File src/gsm/gsup.c: https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c@786 PS2, Line 786: if ((u8 = gsup_msg->cause_rr)) RR cause also have a zero value, see vadim's remark below https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c@789 PS2, Line 789: u8 = gsup_msg->cause_bssap > One wouldn't be able to encode GSM0808_CAUSE_RADIO_INTERFACE_MESSAGE_FAILURE > == 0x00 this way. […] I'm not remembering the details right now ... if it is all on the stack, and if you agree with vadim, then it could be ok to use pointers. But if each cause value would need a dynamically allocated pointer, then rather use bool presence flags. The size of the struct is of no concern, straight forward simplicity to allow encoding zero cause values is the goal. You choose... https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c File src/gsm/gsup_handover.c: https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@37 PS2, Line 37: * Handover extensions for Osmocom GSUP. Take a look at https://osmocom.org/projects/cellular-infrastructure/wiki/Guidelines_for_API_documentation#Files-and-Groups https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@43 PS2, Line 43: * \returns 0 in case of success, negative in case of error (please get in the habit of ending everything with a dot) https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@47 PS2, Line 47: const struct osmo_gsup_an_apdu an_apdu = gsup_msg->an_apdu; (this is actually copying the struct. It's not much, but doesn't seem intentional.) https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@61 PS2, Line 61: uint8_t* buf = msgb_put(msg, an_apdu.data_len); ( "uint8_t *buf" ) https://gerrit.osmocom.org/#/c/12860/2/src/gsm/libosmogsm.map File src/gsm/libosmogsm.map: https://gerrit.osmocom.org/#/c/12860/2/src/gsm/libosmogsm.map@552 PS2, Line 552: osmo_gsup_handover_decode_an_apdu; (why not put it after the end of gsup_sms? thinking alphabetically?) https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c File tests/gsup/gsup_test.c: https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@327 PS2, Line 327: TEST_MSISDN_IE, no msisdn in this msg. The response sends the handover_number back, but the request doesn't include any. It is always MSC-B or MSC-B' sending a handover number to MSC-A. Also below, drop MSISDN everywhere except for the prepareHandover response msg. https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@357 PS2, Line 357: TEST_MSISDN_IE, /* (Handover Number) */ https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@371 PS2, Line 371: TEST_MSISDN_IE, (same, no msisdn here) https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@413 PS2, Line 413: 0x3C, /* OSMO_GSUP_MSGT_E_PREPARE_SEND_END_SIGNAL_REQUEST */ drop the "_PREPARE" https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@415 PS2, Line 415: TEST_MSISDN_IE, no https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@427 PS2, Line 427: 0x3D, /* OSMO_GSUP_MSGT_E_PREPARE_SEND_END_SIGNAL_ERROR */ drop _PREPARE https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@467 PS2, Line 467: TEST_AN_APDU_IE, /* (Handover Detect) */ (would make sense to put Handover Detect above the Handover Complete) https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@471 PS2, Line 471: 0x41, /* OSMO_GSUP_MSGT_E_PROCESS_ACCESS_SIGNALLING_ERROR */ doesn't exist https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@501 PS2, Line 501: 0x45, /* OSMO_GSUP_MSGT_E_FORWARD_ACCESS_SIGNALLING_ERROR */ doesn't exist https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@608 PS2, Line 608: {"E Prepare Send End Signal Request", no "Prepare" below here -- To view, visit https://gerrit.osmocom.org/12860 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic00b0601eacff6d72927cea51767801142ee75db Gerrit-Change-Number: 12860 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Reviewer: daniel <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Comment-Date: Mon, 11 Feb 2019 02:25:48 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
