RestComm / gmlc

Restcomm Location Server
http://www.restcomm.com/
GNU Affero General Public License v3.0
17 stars 37 forks source link

GMLC responseData DialogTimeout #25

Open loayking opened 8 years ago

loayking commented 8 years ago

When deploying GMLC with Global Title 0100 with BCDEvenEncodingScheme in the Sccp Address, the last digit of the GT will be truncated, and this will lead to DialogTimeout because the initiated GT has been modified The solution is configuring Global Title 0100 with BCDOddEncodingScheme in the Sccp Address

deruelle commented 8 years ago

@loayking can you make a pull request with your change as described in our Open Source Playbook ?

vetss commented 8 years ago

Hello @loayking,

thanks, good catch.

But your soulution (https://github.com/RestComm/gmlc/pull/26/files) in also wrong. You can find the proper solution for example in https://github.com/RestComm/smscgateway/blob/master/core/smsc-common-library/src/main/java/org/mobicents/smsc/library/MessageUtil.java see getSccpAddress() method. We have to reuse org.mobicents.protocols.ss7.sccp.parameter.ParameterFactory. This factory selects proper BCDEven/OddEncodingScheme.

loayking commented 8 years ago

Hi Sergery, Yes you are right , I use it direct to test the concept , but yes we have to reuse org.mobicents.protocols.ss7.sccp.parameter.ParameterFactory to selects the proper BCDEven/OddEncodingScheme.

deruelle commented 8 years ago

hi @loayking, were you able to make progress ?

loayking commented 8 years ago

hi @deruelle still working on it, need little more time to follow vetss way

vetss commented 8 years ago

In the method "SccpAddress convertAddressFieldToSCCPAddress(String address)" we need to create SccpAddress not buy code:

GlobalTitle0100 gt = new GlobalTitle0100Impl(address, 0, BCDEvenEncodingScheme.INSTANCE,NumberingPlan.ISDN_TELEPHONY, NatureOfAddress.INTERNATIONAL); return new SccpAddressImpl(RoutingIndicator.ROUTING_BASED_ON_GLOBAL_TITLE, gt, 0,gmlcPropertiesManagement.getHlrSsn());

but by code like: // 1) this part must be invoked onle in "setSbbContext()" method ParameterFactory sccpParameterFact = new ParameterFactory(); // 2) this is code for an Sccp address creating: String address = "...."; NumberingPlan np = NumberingPlan.ISDN_TELEPHONY; NatureOfAddress na = NatureOfAddress.INTERNATIONAL; int ssn = gmlcPropertiesManagement.getHlrSsn(); GlobalTitle gt = sccpParameterFact.createGlobalTitle(address, translationType, np, null, na); return sccpParameterFact.createSccpAddress(RoutingIndicator.ROUTING_BASED_ON_GLOBAL_TITLE, gt, 0, ssn);

vetss commented 8 years ago

Hello @loayking

your update from https://github.com/RestComm/gmlc/pull/27 updates too much code and we can not accept as it is.

I have committed more simple commit: https://github.com/RestComm/gmlc/commit/177b5751b41f1233476da975f726e7dfd0a4c29d

Please check if your issue was fixed.

Thanks for bug reporting and provided code.

loayking commented 8 years ago

Hi @vetss Thanks for your kind feedback, Your update is just like mine, but what I did is take more possibilities to cover all scenarios regarding GlobalTitleIndicator. I also considered to update GMLC properties file with GlobalTitleIndicator status if this is approved. In your commit, if the routing is not base on GT the request will not be sent, but if we make it in configuration, it will be more easy.

BR

vetss commented 8 years ago

Hello @loayking

Firtsly please pay attention that when you are creating your pull request you need to download the last code version (from a master branch) and make your changes as a minimum code change that is needed. Your last pull request contains many format updates and amy be some other changes that are not needed for fixing of this concrete issue. Such "big" update is not possible to be committed because we have a risk to break some logic.

If you need a deeper update (with support of different GTI) fill free to make it as compared with current code (with my last updates). Changes of GMLC properties (GmlcPropertiesManagement) is also possible. This leads also updates fo GmlcPropertiesManagementMBean and updates of CLI and manual part (that should be also provided).

deruelle commented 8 years ago

@vetss shall we add a test case for you commit ?

vetss commented 8 years ago

Hi @deruelle,

such unit test is not a trivial task. I'd like to skip this update unit test if as I understand a fix fit to @loayking demands (please confirm if this, @loayking).

loayking commented 8 years ago

Hi @vetss

actually i did it my way, but yours should do the same.