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.

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

-    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
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("");

sql = ...(".... %S", like)

The same is for DELETE, UPDATE...

+        if (like) octstr_destroy(like);

octstr_destroy will check for NULL for you...

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

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

Thanks,
Alexander Malysh

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>