Re: New Features Question

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