Re: [PATCH] custom MO Parameters for smsc-http
Alexander Malysh <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
now it's perfect ;) would you like to commit or should I commit it? Thanks, Alex Am 09.06.2009 um 16:06 schrieb Alejandro Guerrieri: > "ups" fixed :D > > http://www.blogalex.com/wp-content/uploads/2009/06/kannel-http-mo-params2.patch > > Regards, > -- > Alejandro Guerrieri > [email protected] > > > > On 09/06/2009, at 15:40, Alejandro Guerrieri wrote: > >> 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] >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >>> >> >