Re: Making SMPP esm_class configurable?

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>
> 
>