Vincent, Sometimes dst sent and received back are just in different format (international vs national for instance).
On Wed, Jul 21, 2010 at 11:38 AM, Vincent CHAVANIS <[email protected]>wrote: > > I'm ok for this :| we have no other solution, > but what happens for alias dst ? > we check only for the last 7 digits means you can potentially > receive ucp 53 for 999991234567 instead of 888881234567 right ? > > Vincent. > > > Le 21/07/2010 09:52, Andreas Fink a écrit : > > just dont send two MT's to the same device in the same second. This is the >> fix. delay the second part for 2 sec so they can not get the same timestamp. >> >> On 21.07.2010, at 09:23, Alexander Malysh wrote: >> >> Hi, >>> >>> 2 MT at the same time are just not fixable for EMI due to the protocol >>> flaw... >>> >>> Thanks, >>> Alexander Malysh >>> >>> Am 21.07.2010 um 01:00 schrieb Vincent CHAVANIS: >>> >>> >>>> I'm +0 for this patch >>>> because i really dislike the<LIKE '%searches'>, they are *extremely* >>>> slow >>>> but this is at least the most advanced patch that partially fixes the >>>> EMI's dlr issue. >>>> *but* you still have an issue if 2 MTs are send in the same second (same >>>> ts) to the same destination. >>>> which DLR will you take ? Assuming you're using an 'trans-id' for each >>>> MT, >>>> you have 1/2 (or more) chance to call the wrong one (depending on how >>>> this was inserted on your db) >>>> I personnally experienced this on *hudges* MT pushes. >>>> >>>> Vincent >>>> >>>> >>>> Le 20/07/2010 23:05, Alexander Malysh a écrit : >>>> >>>>> Hi Nikos, >>>>> >>>>> +1 except some indents issues but I will fix it myself... >>>>> >>>>> Any test results from users? >>>>> >>>>> Thanks, >>>>> Alexander Malysh >>>>> >>>>> >>>>> Am 20.07.2010 um 12:07 schrieb Nikos Balkanas: >>>>> >>>>> >>>>> OK. I believe this is it. >>>>>> >>>>>> BR, >>>>>> Nikos >>>>>> ----- Original Message ----- From: "Alexander Malysh"< >>>>>> [email protected]> >>>>>> To: "Nikos Balkanas"<[email protected]> >>>>>> Cc: "Byron Kiourtzoglou"<[email protected]>;<[email protected]> >>>>>> Sent: Tuesday, July 20, 2010 12:14 PM >>>>>> Subject: Re: Patch: EMI UUCP DLRs (final) >>>>>> >>>>>> >>>>>> Hi Nikos, >>>>>> >>>>>> see below ... >>>>>> >>>>>> Am 20.07.2010 um 03:03 schrieb Nikos Balkanas: >>>>>> >>>>>> >>>>>> Hi Alex, >>>>>>> >>>>>>> Please see inlined comments: I am still waiting feedback from some >>>>>>> users testing it. >>>>>>> >>>>>>> >>>>>>> ----- Original Message ----- From: "Alexander Malysh"< >>>>>>>> [email protected]> >>>>>>>> To: "Nikos Balkanas"<[email protected]> >>>>>>>> Cc: "Byron Kiourtzoglou"<[email protected]>;<[email protected]> >>>>>>>> Sent: Tuesday, July 20, 2010 1:14 AM >>>>>>>> Subject: Re: Patch: EMI UUCP DLRs (final) >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> Hi Nikos, >>>>>>>> >>>>>>>> >>>>>>> I'm back from vacation and here are comments to your patch. Patch >>>>>>>> looks OK but still some things to fix: >>>>>>>> >>>>>>>> >>>>>>> --- gw/dlr_mem.c (revision 4833) >>>>>>>> +++ gw/dlr_mem.c (working copy) >>>>>>>> @@ -125,8 +125,28 @@ >>>>>>>> /* XXX: check destination addr too, because e.g. for UCP is not >>>>>>>> enough to check only >>>>>>>> * smsc and timestamp (timestamp is even without >>>>>>>> milliseconds) >>>>>>>> */ >>>>>>>> - if(octstr_compare(dlr->smsc,smsc) == 0&& >>>>>>>> octstr_compare(dlr->timestamp,ts) == 0) >>>>>>>> + if (dst){ >>>>>>>> + Octstr *dst_min; >>>>>>>> + int len1 = octstr_len(dlr->destination), len2 = >>>>>>>> octstr_len(dst); >>>>>>>> + >>>>>>>> + if (len1< len2) >>>>>>>> + return(1); >>>>>>>> + >>>>>>>> + dst_min = octstr_duplicate(dlr->destination); >>>>>>>> + if (len1> len2) >>>>>>>> + octstr_delete(dst_min, 0, len1 - len2); >>>>>>>> + >>>>>>>> + if (octstr_compare(dlr->smsc, smsc) == 0&& >>>>>>>> octstr_compare(dlr-> >>>>>>>> + timestamp, ts) == 0&& octstr_compare(dst_min, dst) == >>>>>>>> 0) { >>>>>>>> + octstr_destroy(dst_min); >>>>>>>> return 0; >>>>>>>> + } >>>>>>>> + octstr_destroy(dst_min); >>>>>>>> + } >>>>>>>> + else >>>>>>>> + if (octstr_compare(dlr->smsc, smsc) == 0&& >>>>>>>> octstr_compare(dlr-> >>>>>>>> + timestamp, ts) == 0) >>>>>>>> + return 0; >>>>>>>> >>>>>>>> >>>>>>> why so complicated? >>>>>>>> if (dst) { >>>>>>>> pos = octstr_len(dlr->destination) - octstr_len(dst); >>>>>>>> if(pos< 0) >>>>>>>> pos = 0; >>>>>>>> if (octstr_compare(dlr->smsc, smsc) == 0&& >>>>>>>> octstr_compare(dlr->timestamp, ts) == 0&& >>>>>>>> octstr_search(dlr->destination, >>>>>>>> dst, pos) !> = -1) >>>>>>>> return 0; >>>>>>>> } else ... >>>>>>>> >>>>>>>> Same degree of complexity. You use octstr_search, I use >>>>>>> octstr_compare. Personally I wouldn't use either, but octstr_search is >>>>>>> slightly faster. The only reason I used octstr_delete& compare is >>>>>>> because >>>>>>> you told me to use truncate on a copy, and I even asked you twice about >>>>>>> it. >>>>>>> Will change it. >>>>>>> >>>>>>> It's not the same. In my version you don't need duplicate, destroy >>>>>> etc. and it just simpler to read... >>>>>> >>>>>> >>>>>> >>>>>>> --- gw/dlr_sdb.c (revision 4833) >>>>>>>> +++ gw/dlr_sdb.c (working copy) >>>>>>>> + if (dst) >>>>>>>> + like = octstr_format("AND \"%S\" LIKE '%%%S' %s", >>>>>>>> fields->field_dst, >>>>>>>> + dst, sdb_get_limit_str()); >>>>>>>> + else >>>>>>>> + like = octstr_imm(sdb_get_limit_str()); >>>>>>>> >>>>>>>> >>>>>>> why not keep sdb_get_limit() in place and do it as for other DBs? >>>>>>>> and \"%S... seems to be wrong... >>>>>>>> >>>>>>>> I don't understand what you are asking for here. The code was like >>>>>>> that before and I didn't change it. \"%S\" is a typo. Will fix. >>>>>>> >>>>>>> why did you put sdb_get_limit_str() here? it should be keep in sql = >>>>>> octstr_format("select ... %s", sdb_get_limit_str, ...)... >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> --- gw/dlr.c (revision 4833) >>>>>>>> +++ gw/dlr.c (working copy) >>>>>>>> Msg *msg = NULL; >>>>>>>> struct dlr_entry *dlr = NULL; >>>>>>>> + Octstr *dst_min = NULL; >>>>>>>> >>>>>>>> >>>>>>> indents.... >>>>>>>> >>>>>>>> >>>>>>> + if (use_dst&& dst) { >>>>>>>> + dst_min = octstr_duplicate(dst); >>>>>>>> + int len = octstr_len(dst); >>>>>>>> + >>>>>>>> + if (len> MIN_DST_LEN) >>>>>>>> + octstr_delete(dst_min, 0, len - MIN_DST_LEN); >>>>>>>> + } >>>>>>>> >>>>>>>> I see. Intends are 3 spaces, instead of 4. That's why I don't use >>>>>>> spaces. I tried my best, but please feel free to adjust them exactly >>>>>>> the way >>>>>>> you like them. >>>>>>> >>>>>>> :) kannel uses 4 spaces instead of tabs... >>>>>> >>>>>> >>>>>> >>>>>>> indents... >>>>>>>> >>>>>>>> >>>>>>> /* destroy struct dlr_entry */ >>>>>>>> dlr_entry_destroy(dlr); >>>>>>>> + octstr_destroy(dst_min); >>>>>>>> >>>>>>>> >>>>>>> ditto >>>>>>>> >>>>>>>> >>>>>>> BR, >>>>>>> Nikos >>>>>>> --- gw/dlr.h (revision 4833) >>>>>>> +++ gw/dlr.h (working copy) >>>>>>> +Msg* dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr >>>>>>> *dst, int type, >>>>>>> + int use_dst); >>>>>>> >>>>>>> change to >>>>>>> +Msg* dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr >>>>>>> *dst, int type, int use_dst); >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Alexander Malysh >>>>>>> >>>>>>> Am 14.07.2010 um 14:38 schrieb Nikos Balkanas: >>>>>>> >>>>>>> >>>>>>> Here it is. Added support for dest also for cimd2 as reported by >>>>>>>> Byron. >>>>>>>> >>>>>>>> I have tested thoroughly dlr_mem.c, but only compilation for DBs, >>>>>>>> since i have no access to them. >>>>>>>> >>>>>>>> @Byron: Could you please test patch for CIMD2 against some of the >>>>>>>> DBs you use? >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Nikos >>>>>>>> ----- Original Message ----- From: "Alexander Malysh"< >>>>>>>> [email protected]> >>>>>>>> To: "Nikos Balkanas"<[email protected]> >>>>>>>> Cc:<[email protected]> >>>>>>>> Sent: Wednesday, July 07, 2010 6:11 PM >>>>>>>> Subject: Re: Patch: EMI UUCP DLRs (final) >>>>>>>> >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> sorry for delay... I have limited inet access now... see answers >>>>>>>> bellow. >>>>>>>> >>>>>>>> Am 30.06.2010 um 19:30 schrieb Nikos Balkanas: >>>>>>>> >>>>>>>> >>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Please see inlined answers. >>>>>>>>> Thanks for the comments and corrections. Please confirm a few >>>>>>>>> remaining choices. >>>>>>>>> >>>>>>>>> BR, >>>>>>>>> Nikos >>>>>>>>> ----- Original Message ----- From: "Alexander Malysh"< >>>>>>>>> [email protected]> >>>>>>>>> To: "Nikos Balkanas"<[email protected]> >>>>>>>>> Cc:<[email protected]> >>>>>>>>> Sent: Wednesday, June 30, 2010 12:02 PM >>>>>>>>> Subject: Re: Patch: EMI UUCP DLRs (final) >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> >>>>>>>>> IMO we don't need to handle no destination case in DLR lookup but >>>>>>>>>> maybe it's not a wrong idea to be able to ignore destination for some >>>>>>>>>> reasons when SMSC send some junk to us. >>>>>>>>>> >>>>>>>>>> Not doing it, could miss some queries altogether. It would still >>>>>>>>> work for the large majority, but could miss a few matches that would >>>>>>>>> have >>>>>>>>> gotten otherwise. >>>>>>>>> >>>>>>>>> this is ok for me, we make it optional... >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> ok here is the review for your patch: >>>>>>>>>> + if (dst){ >>>>>>>>>> + int len = octstr_len(dst); >>>>>>>>>> + char *p = octstr_get_cstr(dst); >>>>>>>>>> >>>>>>>>>> >>>>>>>>> + if (len> MIN_DST_LEN) >>>>>>>>>> + p += len - MIN_DST_LEN; /* get last >>>>>>>>>> MIN_DST_LEN digits */ >>>>>>>>>> + like = octstr_create(strcat("%", p)); >>>>>>>>>> + gwlist_append(binds, like); >>>>>>>>>> + } >>>>>>>>>> >>>>>>>>>> >>>>>>>>> why is this in every dlr implementation? we have abstraction for >>>>>>>>>> this, see dlr.c >>>>>>>>>> >>>>>>>>>> I don't know where in dlr.c you are referring, but I see your >>>>>>>>> reasoning. >>>>>>>>> This is leftover from a previous implementation, where I passed >>>>>>>>> use_dest in the dlr_<DB>. I considered it at the time important to >>>>>>>>> pass the >>>>>>>>> whole dest to dlr_<db> so that debug messages get the whole dest. >>>>>>>>> Since I >>>>>>>>> started passing NULL for dest, this serves no more purpose. I will >>>>>>>>> abstract >>>>>>>>> it in dlr_find and use octstr_delete instead. Will also correct debug >>>>>>>>> messages accordingly in dlr_db.c. >>>>>>>>> >>>>>>>>> you have to abstract it in dlr_find and not repeat the same code >>>>>>>> in each dlr_db driver. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> - sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, >>>>>>>>>> `%S` FROM `%S` WHERE `%S`=? AND `%S`=? LIMIT 1", >>>>>>>>>> + if (dst) >>>>>>>>>> + sql = octstr_format("SELECT `%S`, `%S`, `%S`, `%S`, `%S`, >>>>>>>>>> `%S` FROM `%S` WHERE `%S`=? AND `%S`=? AND `%S`>LIKE ? LIMIT 1", >>>>>>>>>> fields->field_mask, fields->field_serv, >>>>>>>>>> fields->field_url, fields->field_src, >>>>>>>>>> fields->field_dst, fields->field_boxc, >>>>>>>>>> fields->table, fields->field_smsc, >>>>>>>>>> + fields->field_ts, fields->field_dst); >>>>>>>>>> ... >>>>>>>>>> First of all: like ? doesn't work as you expect... it should be >>>>>>>>>> something like: LIKE CONCAT('%%', ?) and >>>>>>>>>> >>>>>>>>>> Thanks. I will look into it. >>>>>>>>> >>>>>>>>> >>>>>>>>> this is too much maintenance for SQL that defined two times, how >>>>>>>>>> about like this: >>>>>>>>>> if (dst) >>>>>>>>>> like = octstr_format('LIKE CONCAT('%%', ?)' ...); >>>>>>>>>> else >>>>>>>>>> like = octstr_create(""); >>>>>>>>>> >>>>>>>>>> You probably mean: >>>>>>>>> >>>>>>>>> like = octstr_create("=?"); >>>>>>>>> >>>>>>>>> no, I mean octstr_create("") or better use octstr_imm("") >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> sql = ...(".... %S", like) >>>>>>>>>> >>>>>>>>>> This is a classical maintenance vs overhead. Malloc is expensive, >>>>>>>>> much more so in Linux than in Solaris. Furthermore, sql="%s%S" is more >>>>>>>>> difficult to read and understand, since SQL mechanism is not >>>>>>>>> explicit, but >>>>>>>>> hidden in variables. Do you really want that? >>>>>>>>> >>>>>>>>> yes, because maintenance is then easier and malloc overhead is in >>>>>>>> linux not so much expensive as you think because glibc has preallocated >>>>>>>> memory pools and not always is system call >>>>>>>> needed. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> The same is for DELETE, UPDATE... >>>>>>>>>> >>>>>>>>>> >>>>>>>>> + if (like) octstr_destroy(like); >>>>>>>>>> >>>>>>>>>> >>>>>>>>> octstr_destroy will check for NULL for you... >>>>>>>>>> >>>>>>>>>> I am aware of that, but it costs a function call and a few more >>>>>>>>> statements...Either way is fine. I can change it. >>>>>>>>> >>>>>>>>> function call is not really issue against code readability... and >>>>>>>> if this is really your argument then convert all calls to if (..!=NULL) >>>>>>>> bla... (just a joke) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> + if (octstr_compare(dlr->smsc, smsc) == 0&& >>>>>>>>>> octstr_compare(dlr-> >>>>>>>>>> + timestamp, ts) == 0&& memcmp(p1 + len1 - size, p2 + >>>>>>>>>> len2 - size, >>>>>>>>>> + size) == 0) >>>>>>>>>> + return 0; >>>>>>>>>> >>>>>>>>>> >>>>>>>>> memcmp??? why not just use truncated destination and do: >>>>>>>>>> octstr_search??? >>>>>>>>>> >>>>>>>>>> Actually octstr_truncate won't work since it truncates from the >>>>>>>>> end. octstr_delete, would work, however, destroying the original >>>>>>>>> Octstr in >>>>>>>>> the process, unless I duplicated them. It would need to be done on >>>>>>>>> both >>>>>>>>> destinations to work. Code would be more, and the malloc, free and >>>>>>>>> copy, >>>>>>>>> have an overhead. Memcmp doesn't change the original Octstr and is >>>>>>>>> natural >>>>>>>>> for such operations. However, it is out of kannel style, so you have >>>>>>>>> every >>>>>>>>> right to ask me to change it. Do you? >>>>>>>>> >>>>>>>>> yes, please change it. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> +Msg *dlr_find(const Octstr *smsc, const Octstr *ts, const Octstr >>>>>>>>>> *dst, int typ, int use_dst) >>>>>>>>>> >>>>>>>>>> >>>>>>>>> you don't need to change function. Just use dst = NULL and check >>>>>>>>>> it. >>>>>>>>>> >>>>>>>>>> No. dst is needed for debug msgs inside dlr_find. Furthermore, >>>>>>>>> use_dst, currently is set only for EMI. Decision is made at driver >>>>>>>>> level so >>>>>>>>> it can easily change to an smsc configuration variable if needed. >>>>>>>>> >>>>>>>>> ok, maybe you are right. What other people think about this ? >>>>>>>> >>>>>>>> >>>>>>>> Am 26.06.2010 um 10:23 schrieb Nikos Balkanas: >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> I believe I have accounted for almost all your comments to produce >>>>>>>>>> the final working patch. I have no way of testing, other than >>>>>>>>>> compilation. >>>>>>>>>> >>>>>>>>>> This patch will povide: >>>>>>>>>> >>>>>>>>>> 1) Align dlr_oracle.c with the rest. Currently, Oracle does a full >>>>>>>>>> dst find/remove/update on each DLR. >>>>>>>>>> 2) Dst use on find/remove/update is controlled in dlr_find by a >>>>>>>>>> single variable, use_dst. This is currently set only at driver level >>>>>>>>>> and >>>>>>>>>> only in the emi driver, while everyone else has it false. However, >>>>>>>>>> very >>>>>>>>>> easily, if need arises, it can be set in smsc configuration. >>>>>>>>>> 3) All DLR handling for all smscs will remain as it used to be >>>>>>>>>> till now. Only emi handling changes, hopefully for the better (that >>>>>>>>>> is the >>>>>>>>>> purpose of the patch :-)). >>>>>>>>>> 4) It will try to match the last 7 digits of the destination or >>>>>>>>>> the length of the destination if it smaller. This is defined in >>>>>>>>>> gw/dlr_p.h >>>>>>>>>> as MIN_DST_LEN. Didn't want to make it larger, wanted to avoid prefix >>>>>>>>>> territory at all costs, since I have seen a lot of mangling there by >>>>>>>>>> the >>>>>>>>>> SMScs. Besides, I believe that 7 digits give enough resolution >>>>>>>>>> 5) People using emi, should rebuild their indeces, especially if >>>>>>>>>> they are running large batch jobs through EMI. The LIKE % >>>>>>>>>> construction is >>>>>>>>>> not very efficient. >>>>>>>>>> >>>>>>>>>> Enjoy, >>>>>>>>>> Nikos >>>>>>>>>> <kannel.diff> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> <kannel.diff> >>>>>>>> >>>>>>>> >>>>>>> <kannel.diff> >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> > >
