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

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.

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