Re: [PATCH] custom MO Parameters for smsc-http

FYI, I've also commited the fix for kannel_receive_sms (removed the  
dlrurl condition).

I've also added test/test_date and utils/decode_emimsg to .cvsignore.

Regards,
--
Alejandro Guerrieri
[email protected]



On 09/06/2009, at 19:05, Alejandro Guerrieri wrote:

> Commited to CVS.
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On 09/06/2009, at 16:30, Alexander Malysh wrote:
>
>> now it's perfect ;)
>>
>> would you like to commit or should I commit it?
>>
>> Thanks,
>> Alex
>>
>> Am 09.06.2009 um 16:06 schrieb Alejandro Guerrieri:
>>
>>> "ups" fixed :D
>>>
>>> http://www.blogalex.com/wp-content/uploads/2009/06/kannel-http-mo-params2.patch
>>>
>>> Regards,
>>> --
>>> Alejandro Guerrieri
>>> [email protected]
>>>
>>>
>>>
>>> On 09/06/2009, at 15:40, Alejandro Guerrieri wrote:
>>>
>>>> Oh, yes, sorry :D
>>>>
>>>> I'd rather duplicate the msg...
>>>> --
>>>> Alejandro Guerrieri
>>>> [email protected]
>>>>
>>>>
>>>>
>>>> On 09/06/2009, at 15:38, Alejandro Guerrieri wrote:
>>>>
>>>>> Yes, I have to move that afterwards, otherwise the  
>>>>> urltrans_fill_escape_codes call would panic:
>>>>>
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: gwlib/octstr.c:2488:  
>>>>> seems_valid_real: Assertion `ostr->len >= 0' failed. (Called  
>>>>> from gwlib/octstr.c:1552:octstr_split_words.)
>>>>> ...
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: 0    
>>>>> bearerbox                           0x0009136d gw_panic + 253
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: 1    
>>>>> bearerbox                           0x000925de log_thread_to + 750
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: 2    
>>>>> bearerbox                           0x0009b360  
>>>>> octstr_split_words + 48
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: 3    
>>>>> bearerbox                           0x000191e2  
>>>>> urltrans_fill_escape_codes + 114
>>>>> 2009-06-09 12:46:13 [15580] [6] PANIC: 4    
>>>>> bearerbox                           0x0005d91b smsc_fake_create  
>>>>> + 21803
>>>>>
>>>>> Probably the bb_smscconn_receive call destroys the msg structure?
>>>>>
>>>>> Regards,
>>>>> --
>>>>> Alejandro Guerrieri
>>>>> [email protected]
>>>>>
>>>>>
>>>>>
>>>>> On 09/06/2009, at 15:16, Alexander Malysh wrote:
>>>>>
>>>>>> only one last thing:
>>>>>>
>>>>>> +        msg->sms.account = octstr_duplicate(account);
>>>>>> +        msg->sms.binfo = octstr_duplicate(binfo);
>>>>>> +        if (ret == -1) {
>>>>>> +            retmsg = octstr_create("Not accepted");
>>>>>> +            retstatus = fm->status_error;
>>>>>> +        } else {
>>>>>> +            retmsg = urltrans_fill_escape_codes(fm- 
>>>>>> >message_sent, msg);
>>>>>> +            retstatus = fm->status_sent;
>>>>>> +        }
>>>>>> +        ret = bb_smscconn_receive(conn, msg);
>>>>>> +    }
>>>>>>
>>>>>> are you sure you want check ret before bb_smscconn_receive ? ;)
>>>>>>
>>>>>> Thanks,
>>>>>> Alex
>>>>>>
>>>>>> Am 09.06.2009 um 13:23 schrieb Alejandro Guerrieri:
>>>>>>
>>>>>>> Ok, done!
>>>>>>>
>>>>>>> http://www.blogalex.com/wp-content/uploads/2009/06/kannel-http-mo-params1.patch
>>>>>>>
>>>>>>> I've fixed the dlr-url bug _only_ on the generic part. If no  
>>>>>>> objections, I'm doing a second patch to fix it on the kannel  
>>>>>>> smsc (since it's in fact a "separate" issue).
>>>>>>>
>>>>>>> I've also fixed a small glitch on the dlrmsg response code (I  
>>>>>>> was using the "error" status code for successful submits as  
>>>>>>> well).
>>>>>>>
>>>>>>> Last but not least, I've added url translation to the response  
>>>>>>> message, so now you can include escape codes on the response,  
>>>>>>> which may come handy on many cases (for example, to return  
>>>>>>> kannel's message id on the requests). Userguide part updated  
>>>>>>> accordingly.
>>>>>>>
>>>>>>> Regards,
>>>>>>> --
>>>>>>> Alejandro Guerrieri
>>>>>>> [email protected]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 09/06/2009, at 11:27, Alexander Malysh wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> Am 09.06.2009 um 11:23 schrieb Alejandro Guerrieri:
>>>>>>>>
>>>>>>>>> Alex,
>>>>>>>>>
>>>>>>>>> Regarding the charsets, I agree it's better to tackle it now  
>>>>>>>>> that leave it for "later" (aka, "never" ;) ).
>>>>>>>>>
>>>>>>>>> I'm fixing the charset issue and the dlr bugs and resending  
>>>>>>>>> later.
>>>>>>>>>
>>>>>>>>
>>>>>>>> thanks alex!
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> --
>>>>>>>>> Alejandro Guerrieri
>>>>>>>>> [email protected]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 09/06/2009, at 11:17, Alexander Malysh wrote:
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Am 09.06.2009 um 11:05 schrieb Alejandro Guerrieri:
>>>>>>>>>>
>>>>>>>>>>> Alex,
>>>>>>>>>>>
>>>>>>>>>>> Commenting inline below.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> --
>>>>>>>>>>> Alejandro Guerrieri
>>>>>>>>>>> [email protected]
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 09/06/2009, at 10:44, Alexander Malysh wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Seems you don't handle charset at all. Do you assume to  
>>>>>>>>>>>> receive UTF-8 in any case? I don't think it's
>>>>>>>>>>>> applicable to generic interface. At least alt-charset  
>>>>>>>>>>>> should be taken in account...
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'm not adding anything new here, the MO handling is built  
>>>>>>>>>>> on the original code for kannel_receive_sms (which was the  
>>>>>>>>>>> default interface for MO on the generic interface anyway).  
>>>>>>>>>>> There's no charset handling in there either. I agree it  
>>>>>>>>>>> makes sense to have charset handling, since an http  
>>>>>>>>>>> interface should be as flexible as possible, but is it a  
>>>>>>>>>>> showstopper at this point?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It is not really show stopper but if it will not be added  
>>>>>>>>>> now it will remain so for ages. It is not a problem for  
>>>>>>>>>> kannel_receive_sms because it's
>>>>>>>>>> well defined interface to smsbox which uses UTF-8.
>>>>>>>>>> I would prefer to see it implemented (there only few line  
>>>>>>>>>> of code ;) )
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> some comments to patch below...
>>>>>>>>>>>>
>>>>>>>>>>>> why is dlrurl required for DLR? We have dlrurl stored in  
>>>>>>>>>>>> DLR storage...
>>>>>>>>>>>>
>>>>>>>>>>>> +    }
>>>>>>>>>>>> +    else if (dlrurl != NULL && dlrmask != 0 && dlrmid !=  
>>>>>>>>>>>> NULL) {
>>>>>>>>>>>> +        /* we got a DLR, and we don't require additional  
>>>>>>>>>>>> values */
>>>>>>>>>>>> +        Msg *dlrmsg;
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Idem... why is it needed for the kannel interface then?
>>>>>>>>>>
>>>>>>>>>> hmm, seems both buggy then. We don't need dlr-url for DLR.  
>>>>>>>>>> Could you please fix both interfaces?
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> hmm this check is not complete. it should be udh_len +  
>>>>>>>>>>>> msgdata_len but msgdata_len depends in coding...
>>>>>>>>>>>>
>>>>>>>>>>>> +    else if (udh != NULL && octstr_len(udh) >  
>>>>>>>>>>>> MAX_SMS_OCTETS) {
>>>>>>>>>>>> +        error(0, "HTTP[%s]: UDH field is too long,  
>>>>>>>>>>>> rejected",
>>>>>>>>>>>> +              octstr_get_cstr(conn->id));
>>>>>>>>>>>> +        retmsg = octstr_create("UDH field is too long,  
>>>>>>>>>>>> rejected");
>>>>>>>>>>>> +        retstatus = fm->status_error;
>>>>>>>>>>>> +    }
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Idem... this is from kannel_receive_sms as well. If it's  
>>>>>>>>>>> wrong here, then it's wrong on the "kannel" smsc as well.
>>>>>>>>>>
>>>>>>>>>> Hmm seems, I'm wrong here. This check checks for the right  
>>>>>>>>>> UDH but allows long MO messages.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> why do you always load generic config options ?
>>>>>>>>>>>>
>>>>>>>>>>>> static void generic_send_sms(SMSCConn *conn, Msg *sms)
>>>>>>>>>>>> {
>>>>>>>>>>>> @@ -1635,6 +1934,7 @@
>>>>>>>>>>>> cfg_get_bool(&conndata->no_sep, cfg, octstr_imm("no-sep"));
>>>>>>>>>>>> conndata->proxy = cfg_get(cfg, octstr_imm("system-id"));
>>>>>>>>>>>> conndata->alt_charset = cfg_get(cfg, octstr_imm("alt- 
>>>>>>>>>>>> charset"));
>>>>>>>>>>>> +    conndata->fieldmap = generic_get_field_map(cfg);
>>>>>>>>>>>>
>>>>>>>>>>>> ---> conndata->fieldmap = NULL:
>>>>>>>>>>>>
>>>>>>>>>>>> if (conndata->send_url == NULL)
>>>>>>>>>>>> panic(0, "HTTP[%s]: Sending not allowed. No 'send-url'  
>>>>>>>>>>>> specified.",
>>>>>>>>>>>> @@ -1693,7 +1993,7 @@
>>>>>>>>>>>>        octstr_get_cstr(conn->id));
>>>>>>>>>>>>  goto error;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> --->    conndata->fieldmap = generic_get_field_map(cfg);
>>>>>>>>>>>>
>>>>>>>>>>>> -        conndata->receive_sms = kannel_receive_sms; /*  
>>>>>>>>>>>> emulate sendsms interface */
>>>>>>>>>>>> +        conndata->receive_sms = generic_receive_sms; /*  
>>>>>>>>>>>> emulate sendsms interface */
>>>>>>>>>>>> conndata->send_sms = generic_send_sms;
>>>>>>>>>>>> conndata->parse_reply = generic_parse_reply;
>>>>>>>>>>>
>>>>>>>>>>> True, it's a waste of CPU cycles if you're using other  
>>>>>>>>>>> smsc-types instead. Fixing it.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Alex
>>>>>>>>>>>>
>>>>>>>>>>>> Am 09.06.2009 um 10:14 schrieb Alejandro Guerrieri:
>>>>>>>>>>>>
>>>>>>>>>>>>> Ok, here's a new version of my patch to add support for  
>>>>>>>>>>>>> custom MO parameters on the generic http-smsc.
>>>>>>>>>>>>>
>>>>>>>>>>>>> New features:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Support for setting the numeric response code for  
>>>>>>>>>>>>> successful and failed requests (as it is now, it always  
>>>>>>>>>>>>> returns 202 HTTP_ACCEPTED).
>>>>>>>>>>>>> 2. Support for setting the text response for successful  
>>>>>>>>>>>>> requests (right now it returns "Sent.").
>>>>>>>>>>>>> 3. Some code cleanups (extra lines, parameter expansion  
>>>>>>>>>>>>> after authorization).
>>>>>>>>>>>>> 4. Documentation.
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://www.blogalex.com/archives/192
>>>>>>>>>>>>>
>>>>>>>>>>>>> I can commit if no objections.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Alejandro Guerrieri
>>>>>>>>>>>>> [email protected]
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 01/06/2009, at 22:50, Alexander Malysh wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> here some comments...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> in generic_receive_sms you first receive all values  
>>>>>>>>>>>>>> than converts it and only
>>>>>>>>>>>>>> after this done check authorization. Make no sense...  
>>>>>>>>>>>>>> first check auth then convert anything.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +    else if (from == NULL || to == NULL || text ==  
>>>>>>>>>>>>>> NULL) {
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +        error(0, "HTTP[%s]: Insufficient args",
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> why extra line?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think retmsg should also be configurable. Because not  
>>>>>>>>>>>>>> all gateways will accept 'Sent'  as response.
>>>>>>>>>>>>>> I think even HTTP error code for not accepted MOs/DLRs  
>>>>>>>>>>>>>> should be configurable.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Alex
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> P.S. And userguide part would be nice ;)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Am 28.05.2009 um 23:00 schrieb Alejandro Guerrieri:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> HI,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I've finished my patch to configure the parameter  
>>>>>>>>>>>>>>> names for MO on the generic http-smsc.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To use it you just add a few entries on the smsc  
>>>>>>>>>>>>>>> definition, only with the parameters you want to rename.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> e.g:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> generic-param-from = "phoneNumber"
>>>>>>>>>>>>>>> generic-param-to = "shortCode"
>>>>>>>>>>>>>>> generic-param-text = "message"
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> More details and the patch here:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://www.blogalex.com/archives/171
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Of course I'm writing the userguide part if this goes  
>>>>>>>>>>>>>>> forward :)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> Alejandro Guerrieri
>>>>>>>>>>>>>>> [email protected]
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>