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

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