The problem is not adding an extra actual parameter.
The problem is that people must now update to the latest Kannel code or
otherwise smppbox won't compile.

== Rene

-----Original Message-----
From: Nikos Balkanas [mailto:[email protected]] 
Sent: Monday, 02 August, 2010 19:29
To: Rene Kluwen; 'Alexander Malysh'
Cc: 'Kannel Devel'
Subject: Re: Patch: EMI UUCP DLRs (final)

Hi Rene,

Why get things unnecessarily complicated? You can update openSMPPbox code 
and leave the new argument always to 0 if you don't need it, or - even 
better - set it to 1 to match against EMI and CMDI DLRs. You can always get 
those in openSMPPbox, since bb can use differential routing.

BR,
Nikos
----- Original Message ----- 
From: "Rene Kluwen" <[email protected]>
To: "'Alexander Malysh'" <[email protected]>; "'Nikos Balkanas'" 
<[email protected]>
Cc: "'Kannel Devel'" <[email protected]>
Sent: Monday, August 02, 2010 2:39 AM
Subject: RE: Patch: EMI UUCP DLRs (final)


>I propose the following patch. It adds
>
> #define DLR_FIND_USE_DST
>
> To dlr.h.
>
> It is a convenient way to know if I should add an extra parameter to
> dlr_find or not. So smppbox (and possibly other gwlib based programs) can
> happily compile with the dlr_find with the extra option and also (if the
> define does not exist) with the "old" dlr_find.
>
> == Rene
>
>
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On Behalf
> Of Alexander Malysh
> Sent: Wednesday, 28 July, 2010 22:59
> To: Nikos Balkanas
> Cc: Kannel Devel
> Subject: Re: Patch: EMI UUCP DLRs (final)
>
> Hi,
>
> patch with minor fixes commited to SVN.
>
> Thanks,
> Alexander Malysh
>
> Am 26.07.2010 um 18:14 schrieb Nikos Balkanas:
>
>> Kyriaco,
>>
>> Previous svn diff I got with -ub, which may be relevant. Try this one
> which is only -u.
>>
>> BR,
>> Nikos
>> ----- Original Message ----- From: "Kyriacos Sakkas"
> <[email protected]>
>> To: "Nikos Balkanas" <[email protected]>
>> Cc: "Kannel Devel" <[email protected]>
>> Sent: Monday, July 26, 2010 5:04 PM
>> Subject: Re: Patch: EMI UUCP DLRs (final)
>>
>>
>>> Hi Nikos,
>>>
>>> I got the svn snapshot today.
>>>
>>> Don't think its an SVN problem, possibly some flag needed for patch.
>>> patch 2.5.9
>>> svn, version 1.4.2 (r22196)
>>> Running on Debian Etch.
>>>
>>> I should be able to hand patch the files where it is giving errors. Some
>>> from what I saw were due to different indentation.
>>>
>>> If you wish I can post the files detailing the rejected patches.
>>>
>>> Kyriacos
>>>
>>>
>>> On 26/07/2010 16:50, Nikos Balkanas wrote:
>>>> Sorry for the irrelevant question. I tried from a fresh downloaded svn:
>>>>
>>>> zdev:~/work/kannel/gateway-> patch < kannel.diff
>>>> Looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> The next patch looks like a unified context diff.
>>>> done
>>>>
>>>> This is from Solaris using sunfreeware's gnu svn:
>>>>
>>>> svn, version 1.6.9 (r901367)
>>>>  compiled Mar 20 2010, 16:02:58
>>>>
>>>> Copyright (C) 2000-2009 CollabNet.
>>>> Subversion is open source software, see http://subversion.tigris.org/
>>>> This product includes software developed by CollabNet
>>>> (http://www.Collab.Net/).
>>>>
>>>> The following repository access (RA) modules are available:
>>>>
>>>> * ra_neon : Module for accessing a repository via WebDAV protocol
>>>> using Neon.
>>>> - handles 'http' scheme
>>>> - handles 'https' scheme
>>>> * ra_svn : Module for accessing a repository using the svn network
>>>> protocol.
>>>> - with Cyrus SASL authentication
>>>> - handles 'svn' scheme
>>>> * ra_local : Module for accessing a repository on local disk.
>>>> - handles 'file' scheme
>>>> * ra_serf : Module for accessing a repository via WebDAV protocol
>>>> using serf.
>>>> - handles 'http' scheme
>>>> - handles 'https' scheme
>>>>
>>>> Hope this helps,
>>>> Nikos
>>>> ----- Original Message ----- From: "Nikos Balkanas"
> <[email protected]>
>>>> To: "Kyriacos Sakkas" <[email protected]>; "Kannel Devel"
>>>> <[email protected]>
>>>> Sent: Monday, July 26, 2010 4:04 PM
>>>> Subject: Re: Patch: EMI UUCP DLRs (final)
>>>>
>>>>
>>>>> Hi Kyriaco,
>>>>>
>>>>> Are you patching against latest svn?
>>>>>
>>>>> BR,
>>>>> Nikos
>>>>> ----- Original Message ----- From: "Kyriacos Sakkas"
>>>>> <[email protected]>
>>>>> To: "Kannel Devel" <[email protected]>
>>>>> Sent: Monday, July 26, 2010 2:08 PM
>>>>> Subject: Re: Patch: EMI UUCP DLRs (final)
>>>>>
>>>>>
>>>>>> Trying to patch to svn downloaded today (same version as in diff) and
>>>>>> getting some errors. I will try to change the diff to work, but id
>>>>>> someone has already done this, please forward,
>>>>>> or let me know whats wrong with my patch command line. Patch file is
>>>>>> from Nikos on the 20th.
>>>>>>
>>>>>> /opt/svn/26072010/trunk# patch --dry-run -p0 -b  < cimd_dlr.diff
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_mysql.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_oracle.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_pgsql.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_mem.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_mssql.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_sdb.c
>>>>>> Hunk #1 FAILED at 212.
>>>>>> Hunk #2 FAILED at 256.
>>>>>> Hunk #3 FAILED at 285.
>>>>>> 3 out of 5 hunks FAILED -- saving rejects to file gw/dlr_sdb.c.rej
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr.c
>>>>>> Hunk #1 FAILED at 371.
>>>>>> 1 out of 4 hunks FAILED -- saving rejects to file gw/dlr.c.rej
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr.h
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/dlr_p.h
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_cimd2.c
>>>>>> Hunk #1 FAILED at 2114.
>>>>>> 1 out of 1 hunk FAILED -- saving rejects to file
>>>>>> gw/smsc/smsc_cimd2.c.rej
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_soap.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_oisd.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_at.c
>>>>>> Hunk #1 succeeded at 2124 with fuzz 1.
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_fake.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_http.c
>>>>>> Hunk #2 succeeded at 931 with fuzz 1.
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_smpp.c
>>>>>> (Stripping trailing CRs from patch.)
>>>>>> patching file gw/smsc/smsc_cgw.c
>>>>>>
>>>>>>
>>>>>> On 26/07/2010 12:40, Kyriacos Sakkas wrote:
>>>>>>> Hi All,
>>>>>>>
>>>>>>> Was away last week too :).
>>>>>>>
>>>>>>> Is this patch already committed to CVS? If not I will patch locally.
>>>>>>> Should be able to start running it tomorrow.
>>>>>>>
>>>>>>> Kyriacos
>>>>>>>
>>>>>>> On 22/07/2010 10:51, Byron Kiourtzoglou wrote:
>>>>>>>
>>>>>>>> Hello Niko,
>>>>>>>>
>>>>>>>> I was away for a short vacation. I will try the patch as soon as
>>>>>>>> possible (but
>>>>>>>> I won't be able to test it until mid next week)
>>>>>>>>
>>>>>>>> BRs
>>>>>>>>
>>>>>>>> Byron
>>>>>>>>
>>>>>>>> On Wednesday 21 July 2010 00:47, Nikos Balkanas wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> Hi Byrona & Kyriaco,
>>>>>>>>>
>>>>>>>>> Did you get the chance to test the patch? The final one submitted
> is
>>>>>>>>> identical in function, except for some cosmetic changes. I am
> mostly
>>>>>>>>> concerned about the correct behaviour of the concat statement in
>>>>>>>>> Mysql &
>>>>>>>>> Oracle, and of course the overall compatibility with EMI & CIMD2,
>>>>>>>>> and SMPP
>>>>>>>>> for positive control.
>>>>>>>>>
>>>>>>>>> BR,
>>>>>>>>> Nikos
>>>>>>>>> ----- Original Message -----
>>>>>>>>> From: "Alexander Malysh" <[email protected]>
>>>>>>>>> To: "Nikos Balkanas" <[email protected]>
>>>>>>>>> Cc: <[email protected]>
>>>>>>>>> Sent: Wednesday, July 21, 2010 12:05 AM
>>>>>>>>> Subject: Re: Patch: EMI UUCP DLRs (final)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Nikos,
>>>>>>>>>
>>>>>>>>> +1 except some indents issues but I will fix it myself...
>>>>>>>>>
>>>>>>>>> Any test results from users?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexander Malysh
>>>>>>>>>
>>>>>>>>> Am 20.07.2010 um 12:07 schrieb Nikos Balkanas:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> OK. I believe this is it.
>>>>>>>>>>
>>>>>>>>>> BR,
>>>>>>>>>> Nikos
>>>>>>>>>> ----- Original Message ----- From: "Alexander Malysh"
>>>>>>>>>> <[email protected]> To: "Nikos Balkanas" <[email protected]>
>>>>>>>>>> Cc: "Byron Kiourtzoglou" <[email protected]>; <[email protected]>
>>>>>>>>>> Sent: Tuesday, July 20, 2010 12:14 PM
>>>>>>>>>> Subject: Re: Patch: EMI UUCP DLRs (final)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Nikos,
>>>>>>>>>>
>>>>>>>>>> see below ...
>>>>>>>>>>
>>>>>>>>>> Am 20.07.2010 um 03:03 schrieb Nikos Balkanas:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> It's not the same. In my version you don't need  duplicate,
>>>>>>>>>> destroy etc.
>>>>>>>>>> and it just simpler to read...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> --- 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> why did you put sdb_get_limit_str() here? it should be keep in
>>>>>>>>>> sql =
>>>>>>>>>> octstr_format("select ... %s", sdb_get_limit_str, ...)...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> --- 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> :) kannel uses 4 spaces instead of tabs...
>>>>>>>>>> :
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> 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>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>> <kannel.diff>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Kyriacos Sakkas
>>>>>> Development Team
>>>>>> Netsmart
>>>>>> Tel: + 357 22 452565
>>>>>> Fax: + 357 22 452566
>>>>>> Email: [email protected]
>>>>>> http://www.netsmart.com.cy
>>>>>>
>>>>>> Taking Business to a New Level!
>>>>>>
>>>>>> ** Confidentiality Notice: The information contained in this email
>>>>>> message may be privileged, confidential and protected from 
>>>>>> disclosure.
>>>>>> If you are not the intended recipient, any dissemination,
> distribution,
>>>>>> or copying of this  email message is strictly prohibited.
>>>>>> If you think that you have received this email message in error,
> please
>>>>>> email the sender at [email protected] **
>>>>>>
>>>>>
>>>
>>>
>>> -- 
>>> Kyriacos Sakkas
>>> Development Team
>>> Netsmart
>>> Tel: + 357 22 452565
>>> Fax: + 357 22 452566
>>> Email: [email protected]
>>> http://www.netsmart.com.cy
>>>
>>> Taking Business to a New Level!
>>>
>>> ** Confidentiality Notice: The information contained in this email
>>> message may be privileged, confidential and protected from disclosure.
>>> If you are not the intended recipient, any dissemination, distribution,
>>> or copying of this  email message is strictly prohibited.
>>> If you think that you have received this email message in error, please
>>> email the sender at [email protected] **
>> <kannel.diff>
>
>
>
> 




Reply via email to