Re: Patch: EMI UUCP DLRs (final)

Hi Alex,

1 more thing:

In the first case it should be:

if (pos < 0)
   return(1);

There is no way that octstr_search would succeed in this case, and you save 
on 3 octstr_compare.

BR,
Nikos
----- Original Message ----- 
From: "Nikos Balkanas" <[email protected]>
To: "Alexander Malysh" <[email protected]>
Cc: "Byron Kiourtzoglou" <[email protected]>; <[email protected]>
Sent: Tuesday, July 20, 2010 4:03 AM
Subject: 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>
>