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

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