Re: Making SMPP esm_class configurable?

Hi Alan,

after the long discussion this patch commited to SVN.

Thanks,
Alexander Malysh

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>