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

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?


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

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