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