[PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Donald Jackson-6
Hi all,

Currently KSMPPD runs a patch against Kannel SVN trunk, I believe it would be better if we could just include this patch in the mainline Kannel as they don't have any negative implications.

Currently the smpp_pdu_unpack() function takes data without length but the smpp_pdu_pack() function provides length so these two functions are not compatible with each other.  This simple patch just allows for compatibility convenience.

Thanks,
Donald

smpp_pdu_pack.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Rene Kluwen

Sure, but why not change KSMPPD to use smpp_pdu_pack() function without the length?

 

== Rene

 

 

Van: devel [mailto:[hidden email]] Namens Donald Jackson
Verzonden: zaterdag 24 september 2016 12:07
Aan: kannel_dev_mailinglist <[hidden email]>
Onderwerp: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

 

Hi all,

 

Currently KSMPPD runs a patch against Kannel SVN trunk, I believe it would be better if we could just include this patch in the mainline Kannel as they don't have any negative implications.

 

Currently the smpp_pdu_unpack() function takes data without length but the smpp_pdu_pack() function provides length so these two functions are not compatible with each other.  This simple patch just allows for compatibility convenience.

 

Thanks,

Donald

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Donald Jackson-6
Yes that how it is currently, just semantically I think it would be better to have all the PDU assembly/disassembly functions in one place.

It may also benefit others in future to have compatible pack/unpack functions out the box.

On Saturday, 24 September 2016, Rene Kluwen <[hidden email]> wrote:

Sure, but why not change KSMPPD to use smpp_pdu_pack() function without the length?

 

== Rene

 

 

Van: devel [mailto:<a href="javascript:_e(%7B%7D,&#39;cvml&#39;,&#39;devel-bounces@kannel.org&#39;);" target="_blank">devel-bounces@kannel.org] Namens Donald Jackson
Verzonden: zaterdag 24 september 2016 12:07
Aan: kannel_dev_mailinglist <<a href="javascript:_e(%7B%7D,&#39;cvml&#39;,&#39;devel@kannel.org&#39;);" target="_blank">devel@...>
Onderwerp: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

 

Hi all,

 

Currently KSMPPD runs a patch against Kannel SVN trunk, I believe it would be better if we could just include this patch in the mainline Kannel as they don't have any negative implications.

 

Currently the smpp_pdu_unpack() function takes data without length but the smpp_pdu_pack() function provides length so these two functions are not compatible with each other.  This simple patch just allows for compatibility convenience.

 

Thanks,

Donald



--
Donald Jackson
http://www.elite-sms-software.com
http://www.ddj.co.za
http://www.thearchitech.com
donald(a)thearchitech.com
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Stipe Tolj-2
In reply to this post by Rene Kluwen
Am 24.09.2016 13:58, schrieb Rene Kluwen:
> Sure, but why not change KSMPPD to use smpp_pdu_pack() function without
> the length?

correct, IF this is something ksmppd needs/wants, then it needs to
handle it on it's side.

What is the purpose of NOT having the length being added?

Stipe

--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Donald Jackson-4
As I say this is semantics, its not entirely necessary.

Usually in library functions they are compatible with each other, eg:

SMPP_PDU *pdu = smpp_pdu_unpack(smpp_pdu_pack(data));

Would work, but in Kannel's case this doesn't work as the one function uses length and the other does not. So the third party user has to now worry about PDU internals to get the functions to work together.



On 26 September 2016 at 19:31, Stipe Tolj <[hidden email]> wrote:
Am <a href="tel:24.09.2016%2013" value="+12409201613" target="_blank">24.09.2016 13:58, schrieb Rene Kluwen:
Sure, but why not change KSMPPD to use smpp_pdu_pack() function without
the length?

correct, IF this is something ksmppd needs/wants, then it needs to handle it on it's side.

What is the purpose of NOT having the length being added?

Stipe

--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------




--
Donald Jackson
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Stipe Tolj-2
Am 26.09.2016 19:38, schrieb Donald Jackson:
> As I say this is semantics, its not entirely necessary.
>
> Usually in library functions they are compatible with each other, eg:
>
> SMPP_PDU *pdu = smpp_pdu_unpack(smpp_pdu_pack(data));
>
> Would work, but in Kannel's case this doesn't work as the one function
> uses length and the other does not. So the third party user has to now
> worry about PDU internals to get the functions to work together.

yeah, but looping in a "use-less" if condition for ever SMPP unpacking,
JUST to make it sweet for the 3rd package is not justified IMO.

I.e. here is what I do in smppbox's gw lib code for duplicating an SMPP
PDU struct:

SMPP_PDU *smpp_pdu_duplicate(Octstr *esme, SMPP_PDU *pdu)
{
        SMPP_PDU *ret = NULL;
        Octstr *os, *os2;
       
        gw_assert(pdu != NULL);
       
        /*
         * We use a kludge here, we pack the PDU
         * then duplicate the packed data and unpack
         * it again to a PDU structure.
         */
        if ((os = SMPP_PDU_PACK(esme, pdu)) != NULL) {
                /* remove first 4 bytes, length indicator */
                os2 = octstr_copy(os, 4, octstr_len(os)-4);
                octstr_destroy(os);
                ret = SMPP_PDU_UNPACK(esme, os2);
                octstr_destroy(os2);
        }

        return ret;
}

Stipe

--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Donald Jackson-4
Fair point, ignore this patch in that case :)

On 26 September 2016 at 19:43, Stipe Tolj <[hidden email]> wrote:
Am <a href="tel:26.09.2016%2019" value="+12609201619" target="_blank">26.09.2016 19:38, schrieb Donald Jackson:
As I say this is semantics, its not entirely necessary.

Usually in library functions they are compatible with each other, eg:

SMPP_PDU *pdu = smpp_pdu_unpack(smpp_pdu_pack(data));

Would work, but in Kannel's case this doesn't work as the one function
uses length and the other does not. So the third party user has to now
worry about PDU internals to get the functions to work together.

yeah, but looping in a "use-less" if condition for ever SMPP unpacking, JUST to make it sweet for the 3rd package is not justified IMO.

I.e. here is what I do in smppbox's gw lib code for duplicating an SMPP PDU struct:

SMPP_PDU *smpp_pdu_duplicate(Octstr *esme, SMPP_PDU *pdu)
{
        SMPP_PDU *ret = NULL;
        Octstr *os, *os2;
       
        gw_assert(pdu != NULL);
       
        /*
         * We use a kludge here, we pack the PDU
         * then duplicate the packed data and unpack
         * it again to a PDU structure.
         */
        if ((os = SMPP_PDU_PACK(esme, pdu)) != NULL) {
                /* remove first 4 bytes, length indicator */
                os2 = octstr_copy(os, 4, octstr_len(os)-4);
                octstr_destroy(os);
                ret = SMPP_PDU_UNPACK(esme, os2);
                octstr_destroy(os2);
        }

        return ret;

}

Stipe

--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------




--
Donald Jackson
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] KSMPPD (and other?) utility methods for smpp_pdu.c

Stipe Tolj-2
Am 26.09.2016 19:47, schrieb Donald Jackson:
> Fair point, ignore this patch in that case :)

np, we're happy that you're active... we still need to get SOMETHING,
before refusing it ;)

Stipe

--
Best Regards,
Stipe Tolj

-------------------------------------------------------------------
Düsseldorf, NRW, Germany

Kannel Foundation                 tolj.org system architecture
http://www.kannel.org/            http://www.tolj.org/

stolj at kannel.org               st at tolj.org
-------------------------------------------------------------------