Change in osmo-msc[master]: Add SGs Interface

[dexter]

dexter has posted comments on this change. ( https://gerrit.osmocom.org/11642 )

Change subject: Add SGs Interface
......................................................................


Patch Set 30:

(96 comments)

I am now through with the review, I hope nothing slipped through, the scrolling in gerrit is very broken at the moment...

Your comments from 01.11 match up with my understanding but I am asking myself now what happens if the subscriber attaches first to 2G/3G (periodic LU timer is running) and then goes to LTE, the SGs LU does not stop such timers but I have the feeling that we need to do exactly that.

Also I think we don't have any TTCN3 test that verifies the periodic LU for 2G. If we do not have that yet we should implement it.

https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/gsm29118.h
File include/osmocom/msc/gsm29118.h:

https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/gsm29118.h@9
PS3, Line 9: 
           : 
           : 
           : 
           : 
> do we really want to have all of those members dynamically allocated on the heap?  I think our gener […]
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h
File include/osmocom/msc/ran_conn.h:

https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h@216
PS29, Line 216: RAN_CONN_USE_UNTRACKED =
> (yes, it is nicer to keep the last comma, so later additions are single-line patches)
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/ran_conn.h@216
PS29, Line 216: RAN_CONN_USE_UNTRACKED =
> Missing comma.
Done


https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/ran_conn.h
File include/osmocom/msc/ran_conn.h:

https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/ran_conn.h@118
PS8, Line 118: 
> ws
Done


https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/ran_conn.h@213
PS8, Line 213: ran_conn_sgs_re
> It looks like this symbols should have been updated after […]
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_iface.h
File include/osmocom/msc/sgs_iface.h:

https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_iface.h@29
PS29, Line 29: 	/* Socket name from osmo_sock_get_name() */
> Here, I guess talloc ownership is clear and straightforward, so it would be ok to keep this as-is, […]
Done


https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/sgs_iface.h
File include/osmocom/msc/sgs_iface.h:

https://gerrit.osmocom.org/#/c/11642/3/include/osmocom/msc/sgs_iface.h@2
PS3, Line 2: 
> we should add coypyright headers to all new files, sorry for not having one in my original patch.
Do we really place copyright notices into header files? I thought the policy here was not to do this? However I checked some header files, some of them have copyright notices some of them do not. I have now added the copyright headers and I would vote to adding copyright notices to all files, regardless of what it is.


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h
File include/osmocom/msc/sgs_server.h:

https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h@1
PS29, Line 1: #pragma once
> closing brace (C), and I guess it should contain 2019
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_server.h@23
PS29, Line 23: 		unsigned int timer[_NUM_SGS_STATE_TIMERS];
> Maybe this name could be "*_SGS_SERVER_*"?
Done


https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/sgs_server.h
File include/osmocom/msc/sgs_server.h:

https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/sgs_server.h@34
PS8, Line 34: 
> const?
this is (should be) set by the VTY.

Unfortunately it was not set before. I have fixed this now It is populated with a default value at first and can be changed using the VTY now. Same is true for the port.


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_vty.h
File include/osmocom/msc/sgs_vty.h:

https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/sgs_vty.h@1
PS29, Line 1: #pragma once
> I think normally we don't have (C) in . […]
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h
File include/osmocom/msc/vlr.h:

https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h@114
PS29, Line 114: 
> Why do we need this? I don't see any references to this struct.
Done


https://gerrit.osmocom.org/#/c/11642/29/include/osmocom/msc/vlr.h@114
PS29, Line 114: 
> the struct is defined 7 lines further down; indeed drop this
Done


https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/vlr.h
File include/osmocom/msc/vlr.h:

https://gerrit.osmocom.org/#/c/11642/8/include/osmocom/msc/vlr.h@196
PS8, Line 196: 
> Please mark it with a FIXME label.
I have removed this. The spec says that the association shall be marked with the cause code on certain events but I could not find any suggestion on how the cause code can be useful later on. Presumably this is intended for logging or diagnosing etc... Should we ever need to keep the cause code we can add it again at any time.


https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/gsm29118.c
File src/libmsc/gsm29118.c:

https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/gsm29118.c@65
PS3, Line 65: 
> could be simplified by msgb_push_u8(msg, msg_type)
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/gsm29118.c
File src/libmsc/gsm29118.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/gsm29118.c@39
PS5, Line 39: 
> Could you add a comment to explain why len is overridden in this specific case? Is this safe?
I have checked this back, its required by the spec. See note in the code.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c
File src/libmsc/gsm_04_08.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c@453
PS29, Line 453: 	conn->vsub->classmark.classmark1 = lu->classmark1;
> In principle, we put the bare msg composition stuff in libosmocore. […]
I think it makes sense to pull this apart, we already have that for BSSMAP and for SGs as well but I think this is in scope for another patch and we should indeed not block this patch with that. Apart from not much is messed up here. There are a lot of other gsm48_create... function in this file.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_04_08.c@455
PS29, Line 455: 
> ws
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_04_08_annotated.c
File src/libmsc/gsm_04_08_annotated.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_04_08_annotated.c@89
PS8, Line 89: 
> There is no such symbol anymore. Why do we need this file BTW? […]
I am sorry, this file shouln't be here. It was just there to keep some notes in the source code without having to mess around in the original file.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c
File src/libmsc/gsm_09_11.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c@347
PS8, Line 347: RV_IND_SMS);
> It seems your editor is doing this alignment automatically. […]
I am not sure, I have now made it look like other function calls in the file. So it should be consistent now.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_09_11.c@350
PS8, Line 350: ee(trans);
> USSD != SMS
See 3GPP TS 29.118 version 15.2.0, 9.4.17 Service indicator. There is only either "CS call indicator" or "SMS indicator" possible. I wonder if USSD is entirely handled by the MME if the mobile is on 4G? and therefore this code path is never executed on SGs Associated?


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c
File src/libmsc/gsm_subscriber.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@58
PS8, Line 58: int subscr_paging_dispatch(unsigned int hooknum, unsigned int event,
> Unrelated change.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@71
PS8, Line 71: 	LOGP(DPAG, LOGL_DEBUG, "Paging %s for %s (event=%d)\n",
> Unrelated change.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@74
PS8, Line 74: 
> Unrelated change.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@80
PS8, Line 80: 	}
> Looks better, but anyway unrelated.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@88
PS8, Line 88: 	/* Inform parts of the system we don't know */
> Same.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@106
PS8, Line 106: 
> Good idea for a separate change.
I have put this in a separate patch.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@144
PS8, Line 144: 
> why?
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@153
PS8, Line 153: 					   gsm_cbfn *cbfn, void *param,
> Unrelated.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@156
PS8, Line 156: 	int rc;
> Unrelated.
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/gsm_subscriber.c@165
PS8, Line 165: 		if (rc <= 0) {
> Unrelated.
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c
File src/libmsc/gsm_subscriber.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@119
PS29, Line 119: SCCP
> Agree, return val info should go to the "API doc" string. […]
This is a static function, so its not part of the API and there shouldn't be a doc-string.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@119
PS29, Line 119: SCCP
> I think this note should precede the function definition.
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@130
PS29, Line 130: 		return sgs_iface_tx_paging(vsub, serv_ind);
> meta question: […]
When the MME performs an LU on the SGs the VLR knows that the subscriber exists and in case of paging it will page through the SGs interface, the MME then does the real paging on the EUTRAN.

Paging is indeed managed on the LTE side, but if an SMS arrives, it will arrive at the MSC, then the MSC needs to know where to deliver the paging. The SGs-Association tells the MSC/VLR that the subscriber is attached on LTE and then sends the paging through the SGs interface to the MME. Those pagings are more or less a command to the MME to trigger the actual paging on LTE.

When the LU happens through the SGs interface, vlr_sgs_fsm.c instead of vlr_lu_fsm.c does the work. I do not start a timer, so I think periodic LU should be disabled. If I am wrong here and the timer is started anyway, then we have a bug. I set the attached flag to be sure that the MSC is acting as if the subscriber were attached normally, so that paging, sms and csfb calls can be done as if the subscriber were on a 2G/3G network.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/gsm_subscriber.c@151
PS29, Line 151:  */
> end with dot
Done


https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/msc_vty.c@615
PS3, Line 615: 
> unrelated notice: Might be worth introducing a function like omso_fsm_inst_state_name(vsup->sgs_fsm) […]
There is already a function with that name. I am using it now.


https://gerrit.osmocom.org/#/c/11642/3/src/libmsc/msc_vty.c@619
PS3, Line 619: 		trans->vsub ? vlr_subscr_name(trans->vsub) : "-",
> no need to break the line here for VTY_NEWLINE
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/msc_vty.c@1429
PS8, Line 1429: 	vty_out(vty, " default-codec tch-h %s%s",
> I think it was already declared in 'sgs_vty.c'.
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/msc_vty.c@705
PS29, Line 705: 			VTY_NEWLINE);
> maybe […]
SGS_UE_ST_NULL is a condition that is interesting to see. SGS_UE_ST_NULL does not imply that the SGs interface is disabled.

A subscriber may go to SGS_UE_ST_NULL and leave it several times throughout its lifetime. Not showing the info when SGS_UE_ST_NULL would be very confusing when debugging things.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/ran_conn.c
File src/libmsc/ran_conn.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/ran_conn.c@394
PS8, Line 394: functions (see above), 
> Clarify please, *what* was already done?
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c
File src/libmsc/sgs_iface.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@95
PS5, Line 95: 		LOGSGC(sgc, LOGL_ERROR, "(sub %s) Connection allocation failed\n", vlr_subscr_msisdn_or_name(vsub));
> spelling: "Cannot"
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@219
PS5, Line 219: 		}
> Could check for sgc->mme->fqdn[0] == '\0' to avoid string traversal in case the string is not empty.
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@307
PS5, Line 307: 	}
> spelling: context-dependent
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@308
PS5, Line 308: 
> This function never fails. […]
That is intended, when the function has a return code it becomes much easier to use. No {} + separate return statement needed. The returned 0 will propagate downwards and tell the main handler that the error has been handled.


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@449
PS5, Line 449: 	/* FIXME: If we are in SGS_UE_ST_NULL while sub->conf_by_radio_contact_ind == false,
> Check return value? This function works like snprintf(): […]
the IMSI is parsed in a central location since it is used so often. I have changed this now. See also sgs_iface_rx()


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@540
PS5, Line 540: 	enum vlr_lu_type type;
> This function never fails. […]
All rx handler functions must have a return code (see sgs_tx_status() above)


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@567
PS5, Line 567: 	if (lu_type_ie[0] == 0x01)
> This function never fails either. […]
(see above)


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@611
PS5, Line 611: 		break;
> Return an error code?
The error is logged, we could return a status to the MME indicating that something is wrong, but technically I think that it is ok just to leave the UDUP out and do nothing since UDUP is, as far as I understood it so far, just the beeping you hear when someone rejects the call.


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_iface.c@782
PS5, Line 782: /* SGsAP-UPLINK-UNITDATA 3GPP TS 29.118, chapter 8.22 */
> Could check for imsi[0] == '\0' to avoid string traversal in case string is not empty.
I don't think that we are that performance critical here, but I added the check. It would help a lot to know the minimum allowed length for an IMSI. The 6 is just a guess.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_iface.c
File src/libmsc/sgs_iface.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_iface.c@1023
PS8, Line 1023: 
> I have never seen such odd alignment o_O. Looks like an alt statement […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c
File src/libmsc/sgs_iface.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@3
PS29, Line 3: /* (C) 2018-2019 by sysmocom s.f.m.c. GmbH
> ) 19
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@98
PS29, Line 98: 
> This is a bad thing to do in general. […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@110
PS29, Line 110: 		conn->complete_layer3_type = COMPLETE_LAYER3_CM_SERVICE_REQ;
> this is not the HLR ... the vlr_subscr_find_by_imsi() does not contact the HLR. […]
I think in this case its just a typo. It should be VLR instead of HLR. When the subscriber is not in the VLR, we can not allocate a connection. An SGs LU has to be done before. Also the callers already checked that the subscriber is indeed there. Its probably missleading that there is an imsi parameter here. Using vsub directly makes more sense.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@121
PS29, Line 121: /* Check if there are connections associated with a given subscriber. If yes,
> instead of another get() here, keep the get() use count from vlr_subscr_find_by_imsi(). […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@277
PS29, Line 277: 		       sgsap_msg_type_name(msg_type), imsi);
> In another VTY related patch I did argue that what you're doing here is safe. […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@329
PS29, Line 329: 		       sgsap_sgs_cause_name(cause));
> this is not so nice, since you're obtaining log context info, even though you don't know whether som […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@348
PS29, Line 348: 	resp = gsm29118_create_status(imsi, cause, msg);
> you are *always* logging an error now. I guess the initial 'if (sgsap_iei < 0)' should have a { .. […]
The errors are handeled, maybe ERROR is to strong and we should log those messages as NOTICE? The return code is fine. 0 just means that the error is handled.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@354
PS29, Line 354:  * reject, depending on the outcome of the location update. */
> [after reading other code] ... […]
Thats correct. "Status" means "Error" in SGs


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@414
PS29, Line 414: 
> (when we're sending MM Info data, is that really a "Request"?)
Yes, at least SGs calls it a "Request", see 3GPP TS 29.118 8.12. Maybe because the VLR is requesting the MME to send the MM information to the UE...


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@454
PS29, Line 454: 
> Of course our VLR does not even have a VLR subscriber stored if it is not attached. […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@597
PS29, Line 597: 
> (a switch() would be nicer here)
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@624
PS29, Line 624: /* SGsAP-EPS-DETACH-INDICATION 3GPP TS 29.118, chapter 8.6 */
> (a switch() would be nicer here)
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@650
PS29, Line 650: 	vlr_sgs_eps_detach(gsm_network->vlr, imsi, type);
> VLR != HLR
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@655
PS29, Line 655: }
> AFAICT this is not sending anything to the HLR
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@708
PS29, Line 708: 
> so an SGsAP Service Request is a reply to an earlier message from the 2G CN?
Yes, its the response to the paging


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@772
PS29, Line 772: 	/* Allocate subscriber connection */
> is it possible that we obtain a conn that was established on GSM and not via SGs? […]
You mean when a subscriber has just created a conn on 2G/3G, then looses reception and the UE does an LU on LTE, resulting in an LU on SGs and then an MO SMS is sent we would get the old conn instead of creating a new one.

Maybe you are right and we are better off if we check if there are lingering conns when either a location update on the SGs happens or when there is an CSFB indication (subsciber just came back to LTE aufer a fallback call). In both cases we shouldn't have connections associated with the VSUB. I have integrated such a check to make won't run into this.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@851
PS29, Line 851: 
> LOGL_ERROR? Also below?
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@865
PS29, Line 865: 
> you are often repeating the pattern "create_status, tx, free". […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@871
PS29, Line 871: 
> GSM23003_IMSI_MIN_DIGITS
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@872
PS29, Line 872: #define TX_STATUS_AND_LOG(sgc, msg_type, cause, fmt) \
> (strlen() is enough, it implies a "[0] == '\0' check")
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@984
PS29, Line 984: 		break;
> others free() after sgs_tx(). One or the other is wrong.
The checks above abort the function early. The msg that comes in needs to be freed before this function exists. The checks here do not free because they have no return statement and the free is done at the bottom. I have restructured it a bit to reduce the amount of msgb_free() here.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@987
PS29, Line 987: 		break;
> ...
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1083
PS29, Line 1083: 		reset_params.vlr_name_present = true;
> (would be nice to also log the Ns11 value)
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1140
PS29, Line 1140: 	},
> inline comment rather without doxygen marker
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_iface.c@1212
PS29, Line 1212: 	 *  the VLR subscriber usage to be balanced. */
> register sgs_ue_fsm?
This is the only FSM we need to register here. sgs_ue_fsm is inside the VLR and everything related to that is done there.


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_server.c
File src/libmsc/sgs_server.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_server.c@50
PS5, Line 50: 		goto out;
> Set rc to -EBADF to allow an outer event loop to detect a closed socket?
I assign -EBADF to rc after each osmo_stream_srv_destroy() now. I hope this is correct now.


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_server.c
File src/libmsc/sgs_server.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_server.c@160
PS8, Line 160: 
> Shouldn't this be configurable from the VTY?
Those are default values which are changed by sgs_server_open() afterwards.

It should be configurable of course. I have fixed it now and there are default values.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_server.c
File src/libmsc/sgs_server.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_server.c@1
PS29, Line 1: /* (C) 2018-2019 by sysmocom s.f.m.c. GmbH
> ) 19
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_vty.c
File src/libmsc/sgs_vty.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/sgs_vty.c@141
PS5, Line 141: 
> Use a #define for the port number?
Done


https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_vty.c
File src/libmsc/sgs_vty.c:

https://gerrit.osmocom.org/#/c/11642/8/src/libmsc/sgs_vty.c@46
PS8, Line 46: }
> Let's don't mix everything into a single line. Hard to read.
I see, I have now split it up. I hope it is now better to read.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c
File src/libmsc/sgs_vty.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@1
PS29, Line 1: /* (C) 2018-2019 by sysmocom s.f.m.c. GmbH
> ) 19
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@99
PS29, Line 99: DEFUN(cfg_sgs_timer, cfg_sgs_timer_cmd,
> have you tested that the keywords are allowed to contain capital letters? […]
I did not know that before. I have tested it now and it indeed causes problems. I have now made it all into small letters.


https://gerrit.osmocom.org/#/c/11642/29/src/libmsc/sgs_vty.c@123
PS29, Line 123: DEFUN(cfg_sgs_counter, cfg_sgs_counter_cmd,
> same capital chars: tested?
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/subscr_conn.c
File src/libmsc/subscr_conn.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libmsc/subscr_conn.c@725
PS5, Line 725: 
> What does 'flag' mean? Try to choose a more expressive name?
Done


https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c
File src/libvlr/vlr_sgs.c:

https://gerrit.osmocom.org/#/c/11642/3/src/libvlr/vlr_sgs.c@91
PS3, Line 91: 	vsub->sgs.paging_cb = paging_cb;
> that kind of printf of course needs to be removed before an eventual merge
I removed this one, but there are others. I am using them as orientation while testing things. I will remove all of them of course before merge.


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c
File src/libvlr/vlr_sgs.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@143
PS5, Line 143: 	case SGSAP_ID_NONEPS_T_COMBINED_UE_EPS_NONEPS:
> Missing 'break;' in default case.
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@174
PS5, Line 174: 		break;
> Missing 'break;' in default case.
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@203
PS5, Line 203: }
> spelling: reception
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@237
PS5, Line 237: 	/* Stop Ts5 and and consider the paging as successful */
> spelling: reception
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs.c@291
PS5, Line 291: 	 * an explicit failure is indicated (MME actively rejects paging).
> Could be simplified to: return osmo_timer_pending(&vsub->sgs. […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c
File src/libvlr/vlr_sgs.c:

https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@89
PS29, Line 89: 	vsub->sgs.cfg = *cfg;
> rather […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@101
PS29, Line 101: 	vsub->cgi.lai = *new_lai;
> please don't use memcpy, but assign members directly. […]
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@205
PS29, Line 205: /*! Notify that an SGs paging has been rejected by the MME.
> wrong comment
Done


https://gerrit.osmocom.org/#/c/11642/29/src/libvlr/vlr_sgs.c@226
PS29, Line 226: /*! Notify that an SGs paging has been accepted by the MME.
> wrong comment
Done


https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c
File src/libvlr/vlr_sgs_fsm.c:

https://gerrit.osmocom.org/#/c/11642/5/src/libvlr/vlr_sgs_fsm.c@197
PS5, Line 197: 		/* In cases where the LU has interrupted the paging, respawn the paging now,
> Missing 'break;' here?
yes, its missing. Thanks.


https://gerrit.osmocom.org/#/c/11642/3/tests/msc_vlr/msc_vlr_test_call.c
File tests/msc_vlr/msc_vlr_test_call.c:

https://gerrit.osmocom.org/#/c/11642/3/tests/msc_vlr/msc_vlr_test_call.c@592
PS3, Line 592: 
> please don't copy+paste this to each test but rather introduce a shared file with those stubs which  […]
Done



-- 
To view, visit https://gerrit.osmocom.org/11642
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I73359925fc1ca72b33a1466e6ac41307f2f0b11d
Gerrit-Change-Number: 11642
Gerrit-PatchSet: 30
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Stefan Sperling <stsp at stsp.name>
Gerrit-CC: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Comment-Date: Tue, 15 Jan 2019 10:51:54 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190115/5a17798e/attachment.htm>