Re: Patch: EMI UUCP DLRs (final)

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.

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

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

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