Re: Making SMPP esm_class configurable?

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 <amalysh@...> 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 <alan@...>
>> 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: devel-bounces@...
>>        [mailto:devel-bounces@...] On
>>> Behalf Of Nikos Balkanas
>>> Sent: Tuesday, 09 August, 2011 02:11
>>> To: Alan McNatty
>>> Cc: devel@...
>>> Subject: Re: Making SMPP esm_class configurable?
>>>
>>>
>>>
>>>
>>> Looks great! +1.
>>>
>>>
>>> Nikos
>>>
>>>
>>>
>>>
>>> On Tue, Aug 9, 2011 at 2:59 AM, Alan McNatty
>>        <alan@...>
>>> 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
>>        <alan@...>
>>>> 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
>>>>        <alan@...>
>>>>> 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"
>>        <amalysh@...>
>>>>>>> To: "Alan McNatty"
>>        <alan@...>
>>>>>>> Cc: "Nikos Balkanas"
>>        <nbalkanas@...>;
>>>>>        <devel@...>
>>>>>>> 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"
>>> <alan@...>
>>>>>>>>>> To: <devel@...>
>>>>>>>>>> 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>