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

[Vadim Yanitskiy]

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

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


Patch Set 32: Code-Review+1

(8 comments)

In general, looks good. Just a few cosmetic issues and more or less important note about SS/USSD.

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

https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/gsm_09_11.c@347
PS32, Line 347: SGSAP_SERV_IND_SMS
Hmm, according to TS 129.118, section 9.4.17, CS call indicator is used for all the CS services except the SMS. So, I think we should use SGSAP_SERV_IND_CS_CALL here.


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

https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/gsm_subscriber.c@134
PS32, Line 134: LOGP
This change seems to be unrelated.


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

https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@66
PS32, Line 66: struct sgs_state;
             : struct sgs_connection;
You're referencing both structures a few lines above, so it looks like we don't need this forward declaration? Otherwise it should have been placed *before* the first reference(s).


https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@113
PS32, Line 113: complete_layer3_type
Alternatively:

  conn->complete_layer3_type = mt ?
    COMPLETE_LAYER3_PAGING_RESP : COMPLETE_LAYER3_CM_SERVICE_REQ;
  ran_conn_update_id(conn);

or you could move ran_conn_update_id() outside the 'if'.


https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@335
PS32, Line 335: vlr_subscr_put
Where is the corresponding vlr_subscr_get()? Why do we need this call?


https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_iface.c@519
PS32, Line 519: osmo_hexdump(err_msg,
(cosmetic) could be moved to the next line, so there would be no need to add such long spacing (\t\t\t\t\t\t...).


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

https://gerrit.osmocom.org/#/c/11642/32/src/libmsc/sgs_server.c@68
PS32, Line 68: /* do we have to notify the SGs code about this? */
Should be marked with TODO or FIXME.


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

https://gerrit.osmocom.org/#/c/11642/32/src/libvlr/vlr_sgs_fsm.c@279
PS32, Line 279: S(SGS_UE_E_RX_LU_FROM_MME) | S(SGS_UE_E_TX_PAGING)
I think it would be better to have each state / event on a separate line, like we do in osmo-bsc. This would improve readability and make the git diff cleaner when adding new states / events...

Example: https://git.osmocom.org/osmo-bsc/tree/src/osmo-bsc/lchan_fsm.c#n1125



-- 
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: 32
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: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-CC: Stefan Sperling <stsp at stsp.name>
Gerrit-Comment-Date: Tue, 22 Jan 2019 10:52:47 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190122/b478dcdb/attachment.htm>