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>
