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 ...
--- 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...
--- 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);
+ }
indents...
/* destroy struct dlr_entry */
dlr_entry_destroy(dlr);
+ octstr_destroy(dst_min);
ditto
--- 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>
