Re: Making SMPP esm_class configurable?

+1

On 9/8/11 9:26 AM, Alejandro Guerrieri wrote:
> Ops, missed the point, sorry.
>
> IMHO restricted to 0 and 3 should cover most cases. You could take it further and allowing 0 _to_ 3 to make it slightly more flexible as well.
>
> Default should be 3 to keep it backwards-compatible.
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On Sep 8, 2011, at 2:17 PM, Alexander Malysh wrote:
>
>> 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>
>>>>
>