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

[Stipe Tolj]

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

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

[Donald Jackson]

Fair point, ignore this patch in that case :)

On 26 September 2016 at 19:43, Stipe Tolj  wrote:

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


-- 
Donald Jackson

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

[Stipe Tolj]

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

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

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



On 26 September 2016 at 19:31, Stipe Tolj  wrote:

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


-- 
Donald Jackson

Re: [PATCH] Reduce logging in dbpool_mysql.c

[Stipe Tolj]

Am 24.09.2016 13:57, schrieb Rene Kluwen:

I don’t agree completely with your reasoning.

Either you want debug information or you don’t.

Enabling debug information yields a lot of information.

What about if you want to debug the mysql pool usage?

Still +0 from me in case it’s redundant info.


both points are viable. I know what Donald means here. He wants DEBUG 
level, but not cluttered with "too much deep" info.


I would suggest wrapping the debug() line into a #ifdef DO_DEBUG which 
CAN be defined in the header section of the source file itself. So 
people CAN easily add it as extra DEBUG level information, but we 
wouldn't see it in the "normal" DEBUG way.


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

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

[Stipe Tolj]

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

Re: [PATCH] Memory leak in smpp_pdu.c

[Stipe Tolj]

Am 24.09.2016 13:55, schrieb Rene Kluwen:

+1 from me.


yep, good catch Donald.

+1 for it.

If no objections, will commit.

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