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>