Hi,
Although subject was started as EMI DLRs, patch addresses both EMi and CIMD2
issue.
I don't think that SMS delivery is an issue. Submission reponse is already
long received and dlr enrtry is already in kannel.
About sending 2 SMS to the same destination at the same time. A very
frequent example is multipart SMS. This is not currently a problem, since
kannel will only generate a dlr for the first part. But this is not proper
behaviour and sometime this should be eventually fixed in kannel.
This patch addressess only the delivery part aka dlr_find. To tackle
simultaneous submission to the same destination, this should be fixed in
smsc2_rout. This is another patch and completely independent to dlr_find. An
easy fix would be to keep last received destination and timestamp and if is
the same put it back to queue and wait at least for 1 s to elapse (no
sleep). A better fix would be to implement a simple dict with key
dest_timestamp and value empty, just to test existense. It should be of
course maintained (discard all old (> 1 s) ) entries. This should be
implemented even for multipart SMS. I can do that, when I get some free
time. Little baby steps at a time ;-)
@Vincent: With respect to destination alias, what do you mean? Looking for
alias in gwlib/cfg.def I cannot find anything relevant.
Many thanks to all people, who contributed ideas and oppinions to the
design. I would like to stress how important early changes to the
conception/design are. Doing all the development and styling (sigh) and then
discovering that you have to redo everything is not a pleasent experience.
Cheers,
Nikos
----- Original Message -----
From: "Alejandro Guerrieri" <[email protected]>
To: "Andreas Fink" <[email protected]>
Cc: "devel Devel" <[email protected]>
Sent: Wednesday, July 21, 2010 12:13 PM
Subject: Re: Patch: EMI UUCP DLRs (final)
This avoid the problem on the first DLR at least (the one Kannel creates on
the submit response), but there's nothing you could do if the SMSC deliver
the messages to the device on the same second and sends you the DLR's as
such (for example if the phone was off/without coverage and was later
available).
Anyway, not much to do about it, it's a protocol flaw really.
Regards,
--
Alejandro Guerrieri
[email protected]
On 21/07/2010, at 09:52, Andreas Fink wrote:
just dont send two MT's to the same device in the same second. This is the
fix. delay the second part for 2 sec so they can not get the same
timestamp.
On 21.07.2010, at 09:23, Alexander Malysh wrote:
Hi,
2 MT at the same time are just not fixable for EMI due to the protocol
flaw...
Thanks,
Alexander Malysh
Am 21.07.2010 um 01:00 schrieb Vincent CHAVANIS:
I'm +0 for this patch
because i really dislike the <LIKE '%searches'>, they are *extremely*
slow
but this is at least the most advanced patch that partially fixes the
EMI's dlr issue.
*but* you still have an issue if 2 MTs are send in the same second (same
ts) to the same destination.
which DLR will you take ? Assuming you're using an 'trans-id' for each
MT,
you have 1/2 (or more) chance to call the wrong one (depending on how
this was inserted on your db)
I personnally experienced this on *hudges* MT pushes.
Vincent
Le 20/07/2010 23:05, Alexander Malysh a ιcrit :
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>
