Re: Making SMPP esm_class configurable?

Hi Alex,

this is not the question whether commit or not. The question is, how patch should looks like?
a) commit as boolean
b) commit as integer with restrictions -> need recompile when not supported values (0 or 3) should be used
c) commit as integer without restrictions

Thanks,
Alexander Malysh

Am 08.09.2011 um 13:37 schrieb Alejandro Guerrieri:

> Having to patch and recompile Kannel a few times in the past to change that parameters to make it work in South America, I can only say +1 to it :)
> 
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
> 
> 
> 
> On Sep 8, 2011, at 12:29 PM, Alexander Malysh wrote:
> 
>> 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>
>> 
>> 
>