Hi,
sorry seems I overlooked your mail :(
Jonathan Houser wrote:
>
> Alex,
>
>> how about somathing like this?
>>
>> #define smpp_pdu(name) \
>> static int smpp_##name_to_msg(SMPP_PDU pdu, ...)
>> {
>> struct name cmd = pdu->u.name;
>> cmd->source_addr = octstr_create(XXX);
>> ...
>> }
>> smpp_pdu(submit_sm);
>> smpp_pdu(data_sm);
>>
>>
>> now to patch: please split your patch in changesets (e.g. add data_sm
>> handling, add new struct members to msg struct, etc...)
>>
>> Thanks in advance!
>
> Finally got some free time to look back at that code again. I see
> kind of what you are doing above, but don't really see how it fixes what
> I'm having an issue with. Could you give a quick example of the
> following code being revamped to work in one equivalent line (so I don't
> have to duplicate code thus making maintenance a nightmare for those who
> come after me)?
>
> ---
>
> if ( /* we are doing submit_sm */ )
> {
> pdu->u.submit_sm.source_addr = octstr_duplicate(msg->sms.sender);
> }
> else if ( /* we are doing data_sm */ )
> {
> pdu->u.data_sm.source_addr = octstr_duplicate(msg->sms.sender);
> }
>
> ---
>
> I mean I understand the structure that is being used above. It
> seems a bit messy to me as you're allocating the same memory for shared
> fields over and over (ie. source_addr is allocated for both submit_sm
> and data_sm), and yet you have to make sure you fill the correct
> instance for the message you're building.
hmm, I don't see a problem with this... pdu struct is a union (means always
fixed size) and only the needed values are filled/allocated.
as to the macros above... the idea is pretty simple, you let compiler
generate 2 functions for submit_sm and data_sm at the same time just
maintaining only one copy of shared operations. so you have something like
this:
static int smpp_data_sm_to_msg_extras(pdu, msg)
{
/* here do some extras that needed only for data_sm */
}
/* shared stuff for submit_sm & data_sm */
#define smpp_pdu(name) \
static int smpp_##name_to_msg(SMPP_PDU pdu, Msg *msg, ...) \
{
struct name cmd = pdu->u.name; \
cmd->source_addr = octstr_duplicate(msg->sms.sender); \
cmd->destination_addr = octstr_duplicate(msg->sms.receiver); \
...
smpp_##name_to_msg_extras(pdu, msg); \
}
smpp_pdu(submit_sm);
smpp_pdu(data_sm);
static int hanle_pdu(...)
{
if (data_sm)
smpp_data_sm_to_msg(pdu, msg);
else
smpp_submit_sm_to_msg(pdu, msg);
}
hope it's clear now...
> That is unless the site I saw
> (not a Kannel site) the structure definition style used by the SMPP code
> was lying to me. :P I mean, if it were a simple struct like I've used
> for everything I've implemented (GSM SMS, IS-41 SMS, MMS, RADIUS), then
> you'd just write the above code as...
>
> ---
>
> pdu.source_addr = octstr_duplicate(msg->sms.sender);
>
> ---
>
> ...and be done with it. If the message you're sending doesn't use
> source_addr, you may needlessly fill it, but the actual 'encoder' will
> not use it to build the message (as it's smart enough to know not to,
> which it would have to be anyway). Needlessly allocated memory ensues
> in that case, but the code stays nice and simple. Perhaps I'm just
> missing something with this allocation style being used?
>
> Jon
--
Thanks,
Alex
