Thanks for your comments. First comment regarding the variable name: The new variable is closely related to status-success-regex which doesn't start with "generic-". But ok, I will change it if you think it's better...
Second comment regarding pmatch[1].rm_eo: The test seems to be useless according to regexec() documentation. But it's cheap so it can't hurt. I will update userguide.xml also. Franck > -----Original Message----- > From: Alejandro Guerrieri [mailto:[email protected]] > Sent: Monday, June 22, 2009 12:37 PM > To: Alexander Malysh > Cc: [email protected];[email protected] > Subject: Re: [PATCH] custom MO Parameters for smsc-http > > +1 with the changes Alex proposed. I'd rename > ack_foreign_id_regex as > well, for clarity's sake. > > Regards, > -- > Alejandro Guerrieri > [email protected] > > On 22/06/2009, at 12:26, Alexander Malysh wrote: > > > Hi, > > > > patch looks ok. Here some minor comments: > > > > + OCTSTR(ack-foreign-id-regex) > > would it be better to name it something like generic-foreign-id- > > regex? so user know it belongs only to generic http... > > > > + if (pmatch[1].rm_so != -1) { > > better to check the end as well -> if (pmatch[1].rm_eo != -1 && > > pmatch[1].rm_so != -1) > > > > Otherwise patch looks good. > > Please make patch to userguide and I will commit it to CVS. > > > > Thanks, > > Alex > > > > Am 19.06.2009 um 14:40 schrieb Franck LAMASUTA: > > > >> Here it is, implemented with second idea... > >> > >> I did a small optimization of previous gw_regex_exec() calls in > >> generic_parse_reply(). > >> I have also fixed 3 debug() calls in generic_receive_sms() > >> ("smsc.http.kannel" => "smsc.http.generic"). > >> > >> Waiting for your comments... > >> > >> Franck > >> > >> > >> > >>> -----Original Message----- > >>> From: Alejandro Guerrieri [mailto:[email protected]] > >>> Sent: Wednesday, June 10, 2009 3:08 PM > >>> To: Alexander Malysh > >>> Cc: [email protected];[email protected] > >>> Subject: Re: [PATCH] custom MO Parameters for smsc-http > >>> > >>> Me too, second one is far more flexible and clean. > >>> -- > >>> Alejandro Guerrieri > >>> [email protected] > >>> > >>> > >>> > >>> On 10/06/2009, at 14:31, Alexander Malysh wrote: > >>> > >>>> Hi, > >>>> > >>>> I'm for the second idea, regex rocks ;) > >>>> > >>>> Thanks, > >>>> Alex > >>>> > >>>> Am 10.06.2009 um 12:37 schrieb Franck LAMASUTA: > >>>> > >>>>> It's not so urgent but I could try, it could be interesting... > >>>>> > >>>>> I have 2 ideas so far... > >>>>> > >>>>> > >>>>> First idea > >>>>> ========== > >>>>> > >>>>> I could add 2 new config parameters to define 2 regex: > >>>>> - the first one, to find the start of the foreign id. > >>>>> - the second one, to find the end of the foreign id. > >>>>> > >>>>> For Claro, they could be defined like this: > >>>>> foreign-id-regex-begin = "<transaction-id>" > >>>>> foreign-id-regex-end = "</transaction-id>" > >>>>> > >>>>> For Clickatell, they could be defined like this: > >>>>> foreign-id-regex-begin = "ID: " > >>>>> foreign-id-regex-end = " " > >>>>> > >>>>> For Brunet, they could be defined like this: > >>>>> foreign-id-regex-begin = "MessageId=" > >>>>> foreign-id-regex-end = " " > >>>>> > >>>>> In generic_parse_reply(), only in case of success, > >>> regex-begin will > >>>>> be used to find the beginning of the id. Starting from > >>> there, regex- > >>>>> end will be used to find the end of the id. If regex-end > >>> can't find > >>>>> a match, the id will be read until the end of text (body) and an > >>>>> ending CRLF will be removed. > >>>>> Then, the id will be used to call dlr_add() (like done in > >>>>> clickatell_parse_reply()). > >>>>> > >>>>> > >>>>> Second idea > >>>>> =========== > >>>>> > >>>>> It's basically the same idea except that only one regex > >>> will be used. > >>>>> > >>>>> For Claro, it could be defined like this: > >>>>> foreign-id-regex = "<transaction-id>(.*)</transaction-id>" > >>>>> > >>>>> For Clickatell, it could be defined like this (if an id contains > >>>>> only digits): > >>>>> foreign-id-regex = "ID: ([0-9]+)" > >>>>> > >>>>> The offsets given by gw_regex_exec() in pmatch[1] could be > >>> used to > >>>>> extract the id. > >>>>> However, it could be more difficult to understand for people who > >>>>> are not regex experts... > >>>>> > >>>>> > >>>>> Any comment, opinion, suggestion or better idea? :) > >>>>> > >>>>> Franck > >>>>> > >>>>> > >>>>> > >>>>> > >>>>>> From: Alexander Malysh > >>>>>> Sent: Tuesday, June 09, 2009 6:05 PM > >>>>>> > >>>>>> > >>>>>> Am 09.06.2009 um 17:55 schrieb Alejandro Guerrieri: > >>>>>> > >>>>>>> Franck, > >>>>>>> > >>>>>>> The dlr part is inherited from the "kannel" smsc > format. In the > >>>>>>> actual implementation there's no way to grab the remote > >>> message id > >>>>>>> from a request, and it'd make a lot of sense to be > able to do > >>>>>>> it. > >>>>>>> > >>>>>>> I can try adding it, but don't refrain to do it so > >>> yourself if you > >>>>>>> feel the urge (you'll learn a lot about the code in the > >>> process!). > >>>>>> > >>>>>> yep, Alex is right here :) and we need more contributors :) > >>>>>> > >>>>>>> > >>>>>>> Opinions? > >>>>>>> -- > >>>>>>> Alejandro Guerrieri > >>>>>>> [email protected] > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 09/06/2009, at 17:47, Franck LAMASUTA wrote: > >>>>>>> > >>>>>>>> Hi Alejandro , > >>>>>>>> > >>>>>>>> I work with Hervé who posted to this list the 25th of May > >>>>>>>> (Claro > >>>>>>>> http communication). > >>>>>>>> It seems the patch you've done on the generic part could be > >>>>>>>> very > >>>>>>>> useful for us to manage our communications with Claro. It > >>>>>> could be > >>>>>>>> better than a specific implementation for Claro. > >>>>>>>> > >>>>>>>> However, I don't understand how your patch manages a DLR: > >>>>>>>> In generic_receive_sms(), you call dlr_find() (line > 1674 of the > >>>>>>>> patched source) to retrieve a previous record stored > in the DLR > >>>>>>>> storage. > >>>>>>>> It seems fine except that dlr_add() is never called in > >>>>>>>> generic_parse_reply(), so I guess dlr_find() will always > >>>>>> return NULL. > >>>>>>>> Am I wrong??? > >>>>>>>> > >>>>>>>> > >>>>>>>> In fact, when we submit a MT SMS to Claro with this request > >>>>>>>> > >>>>>> http://retail.mds.claro.com.br/MSE/api?profile=xxxx&pwd=xxxx&m > >>>>> ode=assync-delivery&ANUM=3333&BNUM=1234567&TEXT=test > >>>>>>>> > >>>>>>>> we get a HTTP code 200 with this body: > >>>>>>>> > >>>>>>>> <?xml version="1.0" encoding="UTF-8" ?> > >>>>>>>> <mse-response> > >>>>>>>> <status-code>0</status-code> > >>>>>>>> <profile>profleID</profile> > >>>>>>>> <transaction-id>1020606201622668099</transaction-id> > >>>>>>>> </mse-response> > >>>>>>>> > >>>>>>>> So I guess that the transaction-id should be used in > >>>>>>>> generic_parse_reply() to call dlr_add(). > >>>>>>>> Unfortunately, the current implementation does not allow > >>>>>> to manage > >>>>>>>> such an id, it only allows to search if the request was > >>>>>> successful > >>>>>>>> or not through the regex. > >>>>>>>> > >>>>>>>> > >>>>>>>> If all my hypothesis are right... :-) > >>>>>>>> I could try to patch generic_parse_reply() to manage > also the > >>>>>>>> id > >>>>>>>> through another regex. It will be my first Kannel patch! ;-) > >>>>>>>> If you prefer to do it, no problem, just let me know. > >>>>>>>> > >>>>>>>> If I'm wrong, please clarify how it works. > >>>>>>>> > >>>>>>>> Regards, > >>>>>>>> Franck > >>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>>> > >>> > >>> > >> <kannel_http-generic.patch> > > > > > > >
