Re: Patch: EMI UUCP DLRs (final)

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.

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

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

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

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

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

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

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>