RE: Making SMPP esm_class configurable?

Alexander

Not sure if you're after comments from people such as myself:  Kannel user, software developer, though not developing Kannel, but able to hack Kannel code for my employer's use.

I very much want this patch.  I had to make the necessary dirty hack to change the esm-class to get Kannel working with our SMSC some time back.  It took quite some time to locate the section of code and to confirm that the change I had made was correct.  (I'm very rusty on C/C++.)  A non-programmer would be intimidated by having to do this.  Furthermore, It has forced us to into a situation where we cannot use later updates without merging them with our branch.

I feel that the supplied values should not be Boolean.  At some point in the future a third option will be wanted; then there will be the problem of supporting old config files with the Boolean values and later config files with some other way of supplying the values.  Consequently I think a non-Boolean solution should be used.

Perhaps accept textual values and *only* the currently supported values; other values cause Panic when loading the config file.  For the currently suggested values:  "Default" and "StoreAndForward".  I believe that text is preferable to integers as it is self-documenting; though integers could be supported as well if desired.  The textual values would simply be converted to their numerical equivalents when then they are loaded.

When the point in time comes to support additional options then it should simply be a matter of accepting a wider range of config values.

Hopefully that makes sense and is useful.

Finally, many thanks to Alan and yourself for the work that has been done with patch.

Regards
Richard


-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Alexander Malysh
Sent: 08 September 2011 11:29
To: Alan McNatty
Cc: [email protected]
Subject: Re: Making SMPP esm_class configurable?

Hi Alan,

thanks for the patch but I'm waiting for more comments in this topic.
If no comments coming, I will commit your patch next week.

Thanks,
Alexander Malysh

Am 07.09.2011 um 02:14 schrieb Alan McNatty:

> Hi Alex,
>
> Based on option a) please find the attached patch. I understand
> reasons for locking it down but it is clear to me that users want more
> control over this setting (beyond what we may think is sensible).
> Userguide patch hopefully spells out a sufficient warning.
>
> Cheers,
> Alan
>
> On Thu, 2011-08-18 at 18:57 +0200, Alexander Malysh wrote:
>> Hi,
>>
>>
>> I don't see this as issue. The user ether let it default or try to
>> change in agreement with SMSC operator. If user will try some random
>> values then operator will just shutdown his account. If you ask me, I
>> don't see any need to allow changing default esm-class. But if we see
>> buggy SMSCs that don't accept STORE & FORWARD mode then I don't know
>> what to expect here. Maybe some drunk developer implement SMS as
>> datagram mode, who knows :-)
>>
>>
>> We have 2 options:
>> a) allow user to set any values and write in the docs that this may
>> be dangerous
>> b) we restrict user to only have two options 0 (default mode) and 3
>> (store & forward), then boolean value should be enough
>>
>>
>> Alex
>>
>>
>> Am 18.08.2011 um 12:33 schrieb Nikos Balkanas:
>>
>>> Hi Alex,
>>>
>>>
>>>
>>> I don't agree that this should be left open to the user. A few may
>>> know what to do with the spec. However, a lot will play around with
>>> the values until they get them working. Consider that a user sets
>>> this to DATAGRAM, and kannel generates a store and forward pdu with
>>> DATAGRAM esm_class. It doesn't hurt kannel, but what about the SMSc
>>> that receives it? It could possibly generate a core dump if the
>>> implementation is widely different, resulting in loss of service. Do
>>> you really want to leave that open?
>>>
>>>
>>> BR,
>>> Nikos
>>>
>>> On Thu, Aug 18, 2011 at 11:31 AM, Alexander Malysh
>>> <[email protected]> wrote:
>>>        Hi Alan,
>>>
>>>        sorry to write my response late (I was on vacation), but I
>>>        don't think we need some restriction on configured
>>>        esm-class.
>>>        If we allow to change esm-class in the config, IMHO there is
>>>        no need to restrict it's value, because
>>>        the user know what he does.
>>>
>>>        For cannel it's equal, which value to set as esm-class
>>>        because kannel doesn't interpreting it.
>>>
>>>        Thanks,
>>>        Alex
>>>
>>>        Am 15.08.2011 um 09:35 schrieb Alan McNatty:
>>>
>>>> Thanks Nikos,
>>>>
>>>> That's good news since, thanks to Edgard for pointing out,
>>>        I forgot to
>>>> include gwlib/cfg.def in the patch. Updated patch
>>>        attached.
>>>>
>>>> Cheers,
>>>> Alan
>>>>
>>>> On Mon, 2011-08-15 at 04:29 +0300, Nikos Balkanas wrote:
>>>>> I guess Alex M is busy. There are a few patches prior to
>>>        yours that
>>>>> wait for commission. Don't worry, he never misses a
>>>        patch. He is the
>>>>> gateway maintainer.
>>>>>
>>>>>
>>>>>
>>>>> BR,
>>>>> Nikos
>>>>>
>>>>> On Mon, Aug 15, 2011 at 3:22 AM, Alan McNatty
>>>        <[email protected]>
>>>>> wrote:
>>>>>       Thanks Rene,
>>>>>
>>>>>       Can anyone with commit access give a final nod of
>>>        acceptance?
>>>>>
>>>>>       On Tue, 2011-08-09 at 21:08 +0200, Rene Kluwen
>>>        wrote:
>>>>>> +1 for me as well.
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: [email protected]
>>>>>       [mailto:[email protected]] On
>>>>>> Behalf Of Nikos Balkanas
>>>>>> Sent: Tuesday, 09 August, 2011 02:11
>>>>>> To: Alan McNatty
>>>>>> Cc: [email protected]
>>>>>> Subject: Re: Making SMPP esm_class configurable?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Looks great! +1.
>>>>>>
>>>>>>
>>>>>> Nikos
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Aug 9, 2011 at 2:59 AM, Alan McNatty
>>>>>       <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>> Thanks again Nikos,
>>>>>>
>>>>>> Yeah 0 and 3 are all I'm interested in - wasn't sure if
>>>>>       others wanted
>>>>>> support for non-compliant things.
>>>>>>
>>>>>> Made changes as you suggested - please check out the
>>>>>       attached patch.
>>>>>>
>>>>>> Cheers,
>>>>>> Alan
>>>>>>
>>>>>> On Mon, 2011-08-08 at 05:39 +0300, Nikos Balkanas wrote:
>>>>>>> Hi Alan,
>>>>>>>
>>>>>>>
>>>>>>> Currently kannel doesn't support more modes. Therefore
>>>>>       test should
>>>>>>> more specific:
>>>>>>>
>>>>>>>
>>>>>>> else if (esm_class != SMPP_STORE... && esm_class !=
>>>>>       DEFAULT...))  //
>>>>>>> use constants
>>>>>>>
>>>>>>>
>>>>>>> Also I see no need for panicking over a single smsc:
>>>>>>>
>>>>>>>
>>>>>>> error(0, "SMPP: Invalid esm_class mode \"5\" in
>>>>>       configuration.
>>>>>>> Switching to \"Store and Forward\");
>>>>>>>
>>>>>>>
>>>>>>> There are still many hexadecimal references to the
>>>>>       userguide, and it
>>>>>>> doesn't restrict choices. Therefore, I suggest the
>>>>>       following text:
>>>>>>>
>>>>>>>
>>>>>>> Value for esm_class according to the SMPP spec.
>>>        Accepted
>>>>>       values are
>>>>>> 0
>>>>>>> (Default smsc mode) and 3 (Store and Forward). Defaults
>>>        to
>>>>>       3.
>>>>>>>
>>>>>>>
>>>>>>> HTH,
>>>>>>> Nikos
>>>>>>>
>>>>>>> On Mon, Aug 8, 2011 at 5:01 AM, Alan McNatty
>>>>>       <[email protected]>
>>>>>>> wrote:
>>>>>>>       Thanks Nikos,
>>>>>>>
>>>>>>>       See attached.
>>>>>>>
>>>>>>>       Also just wanted to check thoughts the range
>>>        check
>>>>>       (possibly
>>>>>>>       remove and
>>>>>>>       leave it open?).
>>>>>>>
>>>>>>>       i.e.
>>>>>>>       +    else if (esm_class > 0x03 || esm_class < 0)
>>>>>>>       +        panic(0, "SMPP: Invalid esm_class
>>>>>       directive in
>>>>>>>       configuration.");
>>>>>>>
>>>>>>>
>>>>>>>       On Mon, 2011-08-08 at 04:44 +0300, Nikos
>>>        Balkanas
>>>>>       wrote:
>>>>>>>> Hi Alan,
>>>>>>>>
>>>>>>>>
>>>>>>>> Patch looks good. userguide needs some changes:
>>>>>>>>
>>>>>>>>
>>>>>>>> 1) Capitalize after periods (For example, For
>>>>>       default
>>>>>> mode)
>>>>>>>> 2) In configuration the value should be integer,
>>>>>       not hex
>>>>>> (3
>>>>>>>       not 03).
>>>>>>>> cfg_get_integer doesn't understand hex (0xA5).
>>>>>>>>
>>>>>>>>
>>>>>>>> +1
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Nikos
>>>>>>>>
>>>>>>>> On Mon, Aug 8, 2011 at 4:02 AM, Alan McNatty
>>>>>>>       <[email protected]>
>>>>>>>> wrote:
>>>>>>>>       patch attached.
>>>>>>>>
>>>>>>>>       Votes / comments, etc?
>>>>>>>>
>>>>>>>>       On Wed, 2011-08-03 at 09:39 +1200, Alan
>>>>>       McNatty
>>>>>>>       wrote:
>>>>>>>>> Thanks Nikos/Alex for the feedback - I
>>>>>       will work
>>>>>>>       on config
>>>>>>>>       patch.
>>>>>>>>>
>>>>>>>>> On Tue, 2011-08-02 at 23:10 +0300,
>>>>>       Nikos
>>>>>> Balkanas
>>>>>>>       wrote:
>>>>>>>>>> Hi Alan,
>>>>>>>>>>
>>>>>>>>>> Just to clarify on what Alex says.
>>>>>       Some other
>>>>>>>       modes that
>>>>>>>>       the SMSc may
>>>>>>>>>> support in default mode, are:
>>>>>>>>>>
>>>>>>>>>> Datagram: UDP based, immediate best
>>>>>       effort
>>>>>> high
>>>>>>>       throughput
>>>>>>>>       transmition with
>>>>>>>>>> no retried, validity period or
>>>>>       storage.
>>>>>> Similar
>>>>>>>       to UDP.
>>>>>>>>>> Forward: Single transaction based,
>>>>>       for
>>>>>> real-time
>>>>>>>>       applications, i.e. parking
>>>>>>>>>> tickets, without storage, where
>>>>>       result is
>>>>>>>       returned in
>>>>>>>>       response.
>>>>>>>>>>
>>>>>>>>>> Kannel doesn't support those, only
>>>>>       reliable
>>>>>>>       store and
>>>>>>>>       forward. Therefore the
>>>>>>>>>> default mode wouldn't be
>>>>>       appropriate.
>>>>>>>>>> Configuration would be fine for
>>>>>       those buggy
>>>>>>>       SMScs, that do
>>>>>>>>       store and
>>>>>>>>>> forward, but do not accept it as an
>>>>>       option.
>>>>>>>>>>
>>>>>>>>>> BR,
>>>>>>>>>> Nikos
>>>>>>>>>>
>>>>>>>>>> ----- Original Message -----
>>>>>>>>>> From: "Alexander Malysh"
>>>>>       <[email protected]>
>>>>>>>>>> To: "Alan McNatty"
>>>>>       <[email protected]>
>>>>>>>>>> Cc: "Nikos Balkanas"
>>>>>       <[email protected]>;
>>>>>>>>       <[email protected]>
>>>>>>>>>> Sent: Tuesday, August 02, 2011 12:29
>>>>>       PM
>>>>>>>>>> Subject: Re: Making SMPP esm_class
>>>>>> configurable?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> please don't change default because
>>>>>       we want
>>>>>> that
>>>>>>>       SMSC
>>>>>>>>       _store_ and _forward_
>>>>>>>>>> our message that
>>>>>>>>>> is what we also tell SMSC. This
>>>>>       works in 99%
>>>>>>>       cases but
>>>>>>>>       sometimes buggy SMSCs
>>>>>>>>>> don't accept this
>>>>>>>>>> and rejects messages.
>>>>>>>>>>
>>>>>>>>>> Please keep default as is and make
>>>>>       config
>>>>>> option
>>>>>>>       for buggy
>>>>>>>>       SMSCs.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Alex
>>>>>>>>>>
>>>>>>>>>> Am 02.08.2011 um 06:11 schrieb Alan
>>>>>       McNatty:
>>>>>>>>>>
>>>>>>>>>>> Sorry that should be
>>>>>>>       ESM_CLASS_SUBMIT_DEFAULT_SMSC_MODE.
>>>>>>>>>>>
>>>>>>>>>>> Index: gw/smsc/smsc_smpp.c
>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>> ===================================================================
>>>>>>>>>>> --- gw/smsc/smsc_smpp.c (revision
>>>>>       4913)
>>>>>>>>>>> +++ gw/smsc/smsc_smpp.c (working
>>>>>       copy)
>>>>>>>>>>> @@ -876,7 +876,7 @@
>>>>>>>>>>>    * set the esm_class field
>>>>>>>>>>>    * default is store and
>>>>>       forward, plus
>>>>>> udh
>>>>>>>       and rpi if
>>>>>>>>       requested
>>>>>>>>>>>    */
>>>>>>>>>>> -    pdu->u.submit_sm.esm_class =
>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_STORE_AND_FORWARD_MODE;
>>>>>>>>>>> +    pdu->u.submit_sm.esm_class =
>>>>>>>>       ESM_CLASS_SUBMIT_DEFAULT_SMSC_MODE;
>>>>>>>>>>>   if
>>>>>       (octstr_len(msg->sms.udhdata))
>>>>>>>>>>>       pdu->u.submit_sm.esm_class
>>>>>       =
>>>>>>>>       pdu->u.submit_sm.esm_class |
>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_UDH_INDICATOR;
>>>>>>>>>>>
>>>>>>>>>>> On Tue, 2011-08-02 at 15:59 +1200,
>>>>>       Alan
>>>>>>>       McNatty wrote:
>>>>>>>>>>>> Hi Nikos,
>>>>>>>>>>>>
>>>>>>>>>>>> You mean simply change the
>>>>>       default:
>>>>>>>>>>>>
>>>>>>>>>>>> Index: gw/smsc/smsc_smpp.c
>>>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>> ===================================================================
>>>>>>>>>>>> --- gw/smsc/smsc_smpp.c (revision
>>>>>       4913)
>>>>>>>>>>>> +++ gw/smsc/smsc_smpp.c (working
>>>>>       copy)
>>>>>>>>>>>> @@ -876,7 +876,7 @@
>>>>>>>>>>>>    * set the esm_class field
>>>>>>>>>>>>    * default is store and
>>>>>       forward, plus
>>>>>> udh
>>>>>>>       and rpi
>>>>>>>>       if requested
>>>>>>>>>>>>    */
>>>>>>>>>>>> -    pdu->u.submit_sm.esm_class =
>>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_STORE_AND_FORWARD_MODE;
>>>>>>>>>>>> +    pdu->u.submit_sm.esm_class =
>>>>>>>>       ESM_CLASS_DEFAULT_SMSC_MODE;
>>>>>>>>>>>>   if
>>>>>       (octstr_len(msg->sms.udhdata))
>>>>>>>>>>>>
>>>>>       pdu->u.submit_sm.esm_class =
>>>>>>>>       pdu->u.submit_sm.esm_class |
>>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_UDH_INDICATOR;
>>>>>>>>>>>>
>>>>>>>>>>>> Anyone think we should have a
>>>>>       config
>>>>>> option?
>>>>>>>       Or just
>>>>>>>>       happy to run with
>>>>>>>>>>>> he above. I need to test myself
>>>>>       but is this
>>>>>>>       likely to
>>>>>>>>       be a compatibility
>>>>>>>>>>>> breaker for anyone?
>>>>>>>>>>>>
>>>>>>>>>>>> Cheers,
>>>>>>>>>>>> Alan
>>>>>>>>>>>>
>>>>>>>>>>>> On Mon, 2011-08-01 at 07:13
>>>>>       +0300, Nikos
>>>>>>>       Balkanas
>>>>>>>>       wrote:
>>>>>>>>>>>>> Hi Alan,
>>>>>>>>>>>>>
>>>>>>>>>>>>> According to the spec SMPP 5.0,
>>>>>       p 125,
>>>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_DEFAULT_SMSC_MODE is
>>>>>>>>>>>>> the default esm class. That part
>>>>>       should be
>>>>>>>       patched in.
>>>>>>>>       As far as making
>>>>>>>>>>>>> it
>>>>>>>>>>>>> configurable, I have no
>>>>>       objections to it.
>>>>>> A
>>>>>>>       few people
>>>>>>>>       over the years
>>>>>>>>>>>>> have
>>>>>>>>>>>>> had to manually patch it in.
>>>>>>>>>>>>>
>>>>>>>>>>>>> BR,
>>>>>>>>>>>>> Nikos
>>>>>>>>>>>>> ----- Original Message -----
>>>>>>>>>>>>> From: "Alan McNatty"
>>>>>> <[email protected]>
>>>>>>>>>>>>> To: <[email protected]>
>>>>>>>>>>>>> Sent: Monday, August 01, 2011
>>>>>       6:21 AM
>>>>>>>>>>>>> Subject: Making SMPP esm_class
>>>>>> configurable?
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I found a thread on this from
>>>>>       back in Feb
>>>>>>>       2005
>>>>>>>>       (having received a query
>>>>>>>>>>>>>> from provided now myself) ..
>>>>>       last word by
>>>>>>>       Alejandro
>>>>>>>>       and a lukewarm
>>>>>>>>>>>>>> (+0 -
>>>>>>>>>>>>>> +1) comment from Stipe about
>>>>>       committing
>>>>>> if
>>>>>>>       patch
>>>>>>>>       provided. I would
>>>>>>>>>>>>>> provide a config patch if
>>>>>       anyone would
>>>>>> vote
>>>>>>>       in it's
>>>>>>>>       favour?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Consider:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> gw/smsc/smsc_smpp.c
>>>>>>>>>>>>>> 875     /*
>>>>>>>>>>>>>> 876      * set the esm_class
>>>>>       field
>>>>>>>>>>>>>> 877      * default is store and
>>>>>       forward,
>>>>>>>       plus udh and
>>>>>>>>       rpi if requested
>>>>>>>>>>>>>> 878      */
>>>>>>>>>>>>>> 879
>>>>>       pdu->u.submit_sm.esm_class =
>>>>>>>>>>>>>>
>>>>>       ESM_CLASS_SUBMIT_STORE_AND_FORWARD_MODE;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> But the 'default' is surely
>>>>>>>>       ESM_CLASS_SUBMIT_DEFAULT_SMSC_MODE, no?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>> Alan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> <esm_class.patch>
>>>
>>>
>>
>>
>
> <esm_class.patch2>


Regards, 

Richard Crawshaw
Software Developer

Contact Group

Telephone (UK): 0844 448 442
Telephone (ROI): 0818 286 795
Mobile : 0786 625 9418
Direct Facsimile: 0844 448 9090
Email: [email protected]

Web Site: http://www.contactgroup.com/
Twitter: http://twitter.com/@contactgroup

This e-mail is intended for the above named only. It may contain private and confidential information or material that is 
privileged. If it has come to you in error you must take no action based upon it, nor must you copy or show it to anyone. 
Please return the message immediately to [email protected]
The Validity of any pricing information contained herein is 30 days unless otherwise stated. E & OE. Standard 
terms and conditions apply. Service by Contact Group. Registered in England No. 4125665. Registered office: 
Lonsdale House, 52 Blucher Street, Birmingham, B1 1QU. Data Protection No. Z7911829