<p style="white-space: pre-wrap; word-wrap: break-word;">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).</p><p><a href="https://gerrit.osmocom.org/12860">View Change</a></p><p>24 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h">File include/osmocom/gsm/gsup.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@191">Patch Set #2, Line 191:</a> <code style="font-family:monospace,monospace">   OSMO_GSUP_MSGT_E_PROCESS_ACCESS_SIGNALLING_ERROR        = 0b01000010,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the Process Access Signalling and Forward Access Signalling will never return an Error.<br>I know there was some obscure reason, but can't we get around having to define them?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@196">Patch Set #2, Line 196:</a> <code style="font-family:monospace,monospace">OSMO_GSUP_MSGT_E_CLOSE</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">As far as I can see, this comes from TCAP. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">actually, I did model an "abort" message in my osmo-msc/doc/interMSC_HO_GSUP_msgs.txt, and ...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@197">Patch Set #2, Line 197:</a> <code style="font-family:monospace,monospace">    OSMO_GSUP_MSGT_E_ABORT                                  = 0b01001011,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">... and here is an Abort message. @vadim, what do you mean?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@338">Patch Set #2, Line 338:</a> <code style="font-family:monospace,monospace">an_apdu</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would use a pointer here. The osmo_gsup_message is already quite big...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">size is IMHO not a good reason. Is there another one? Otherwise, no need to extract two ints and a pointer.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup.h@341">Patch Set #2, Line 341:</a> <code style="font-family:monospace,monospace">cause_sm</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does SM mean "Short Message"? If yes, we already have "sm_rp_cause".</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">apparently means "Session Management" :(<br>gsm48_gsm_cause in gsm_04_08_gprs.h says: "definition in 3GPP TS 24.008 10.5.6.6 / Table 10.5.157"<br>I was going to call it cause_rr or something, but the spec does have this as a name.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h">File include/osmocom/gsm/gsup_handover.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h@19">Patch Set #2, Line 19:</a> <code style="font-family:monospace,monospace">uint8_t</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">const?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">agree</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/include/osmocom/gsm/gsup_handover.h@20">Patch Set #2, Line 20:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if this is all there is to this API, then let's just place these in the common gsup.[hc]?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c">File src/gsm/gsup.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c@786">Patch Set #2, Line 786:</a> <code style="font-family:monospace,monospace">    if ((u8 = gsup_msg->cause_rr))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">RR cause also have a zero value, see vadim's remark below</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup.c@789">Patch Set #2, Line 789:</a> <code style="font-family:monospace,monospace">u8 = gsup_msg->cause_bssap</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">One wouldn't be able to encode GSM0808_CAUSE_RADIO_INTERFACE_MESSAGE_FAILURE == 0x00 this way. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The size of the struct is of no concern, straight forward simplicity to allow encoding zero cause values is the goal. You choose...</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c">File src/gsm/gsup_handover.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@37">Patch Set #2, Line 37:</a> <code style="font-family:monospace,monospace"> *  Handover extensions for Osmocom GSUP.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Take a look at https://osmocom.org/projects/cellular-infrastructure/wiki/Guidelines_for_API_documentation#Files-and-Groups</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@43">Patch Set #2, Line 43:</a> <code style="font-family:monospace,monospace"> * \returns 0 in case of success, negative in case of error</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(please get in the habit of ending everything with a dot)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@47">Patch Set #2, Line 47:</a> <code style="font-family:monospace,monospace">      const struct osmo_gsup_an_apdu an_apdu = gsup_msg->an_apdu;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(this is actually copying the struct. It's not much, but doesn't seem intentional.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/gsup_handover.c@61">Patch Set #2, Line 61:</a> <code style="font-family:monospace,monospace">    uint8_t* buf = msgb_put(msg, an_apdu.data_len);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">( "uint8_t *buf" )</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/libosmogsm.map">File src/gsm/libosmogsm.map:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/src/gsm/libosmogsm.map@552">Patch Set #2, Line 552:</a> <code style="font-family:monospace,monospace">osmo_gsup_handover_decode_an_apdu;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(why not put it after the end of gsup_sms? thinking alphabetically?)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c">File tests/gsup/gsup_test.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@327">Patch Set #2, Line 327:</a> <code style="font-family:monospace,monospace">               TEST_MSISDN_IE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no msisdn in this msg. The response sends the handover_number back, but the request doesn't include any.<br>It is always MSC-B or MSC-B' sending a handover number to MSC-A.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also below, drop MSISDN everywhere except for the prepareHandover response msg.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@357">Patch Set #2, Line 357:</a> <code style="font-family:monospace,monospace">         TEST_MSISDN_IE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">/* (Handover Number) */</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@371">Patch Set #2, Line 371:</a> <code style="font-family:monospace,monospace">              TEST_MSISDN_IE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(same, no msisdn here)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@413">Patch Set #2, Line 413:</a> <code style="font-family:monospace,monospace">               0x3C, /* OSMO_GSUP_MSGT_E_PREPARE_SEND_END_SIGNAL_REQUEST */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">drop the "_PREPARE"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@415">Patch Set #2, Line 415:</a> <code style="font-family:monospace,monospace">           TEST_MSISDN_IE,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@427">Patch Set #2, Line 427:</a> <code style="font-family:monospace,monospace">           0x3D, /* OSMO_GSUP_MSGT_E_PREPARE_SEND_END_SIGNAL_ERROR */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">drop _PREPARE</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@467">Patch Set #2, Line 467:</a> <code style="font-family:monospace,monospace">             TEST_AN_APDU_IE, /* (Handover Detect) */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(would make sense to put Handover Detect above the Handover Complete)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@471">Patch Set #2, Line 471:</a> <code style="font-family:monospace,monospace">               0x41, /* OSMO_GSUP_MSGT_E_PROCESS_ACCESS_SIGNALLING_ERROR */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">doesn't exist</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@501">Patch Set #2, Line 501:</a> <code style="font-family:monospace,monospace">               0x45, /* OSMO_GSUP_MSGT_E_FORWARD_ACCESS_SIGNALLING_ERROR */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">doesn't exist</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12860/2/tests/gsup/gsup_test.c@608">Patch Set #2, Line 608:</a> <code style="font-family:monospace,monospace">               {"E Prepare Send End Signal Request",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no "Prepare" below here</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12860">change 12860</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/12860"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ic00b0601eacff6d72927cea51767801142ee75db </div>
<div style="display:none"> Gerrit-Change-Number: 12860 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 11 Feb 2019 02:25:48 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>