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

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