This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgNeels 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 <osmith at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Comment-Date: Mon, 11 Feb 2019 02:25:48 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190211/58aab22d/attachment.htm>