Re: [PATCH] No PANIC on failing store_load()
Hi Rene, +1 if you will use sms_type enum instead of hardcoded values. Alex Am 17.09.2013 um 18:56 schrieb Rene Kluwen rene.klu...@chimit.nl: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. == Rene -Original Message- From: devel [mailto:devel-boun...@kannel.org] On Behalf Of Stipe Tolj Sent: dinsdag 17 september 2013 17:55 Cc: kannel_dev_mailinglist devel@kannel. org Subject: Re: [PATCH] No PANIC on failing store_load() Am 17.09.2013 17:22, schrieb Willy Mularto: Sorry, it doesn't mean setting default value during table creation right? Because this approach will still might be interupted by user's value. Thanks. The default value is defined during table creation, yes. The point is: IF the user doesn't insert a specific value for the field THEN the default is applied. If the user specifically inserts -1 then he will get -1. But that makes no logical sense. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- default.diff
RE: [PATCH] No PANIC on failing store_load()
Also +1. A common error when using sqlbox is not setting sms_type to the proper value (2) whilst inserting an MT to send_sms. This will probably fill the store with messages with invalid sms_type values. == Rene -Original Message- From: devel [mailto:devel-boun...@kannel.org] On Behalf Of Willy Mularto Sent: dinsdag 17 september 2013 3:06 To: Alejandro Guerrieri Cc: devel@kannel.org; Stipe Tolj Subject: Re: [PATCH] No PANIC on failing store_load() +1 It will be good to dump it to bearerbox-access log and flag it as FAILED MT and maybe add more info into it so we can analyse it. Thanks Stipe. On Sep 16, 2013, at 11:56 PM, Alejandro Guerrieri alejandro.guerri...@gmail.com wrote: +1 this was really an annoyance IMHO -- Alejandro Guerrieri On Sep 16, 2013, at 11:59 AM, Stipe Tolj st...@kannel.org wrote: Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- store_load_no_panic.diff
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 15:42, schrieb Rene Kluwen: Also +1. A common error when using sqlbox is not setting sms_type to the proper value (2) whilst inserting an MT to send_sms. This will probably fill the store with messages with invalid sms_type values. yep, exactly. Can't we use default for the row entries for values that are not explicitly set by the INSERT statement? Not sure how standardized the CREATE table statement is regarding the default of values across the DBMS we support. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 03:06, schrieb Willy Mularto: +1 It will be good to dump it to bearerbox-access log and flag it as FAILED MT and maybe add more info into it so we can analyse it. Thanks Stipe. BTW, committed to svn trunk. Disagree here for the FAILED SMS access-log entry, since it is logically not failing. The SMSC *HAS* accepted the submit_sm PDU, since we get a submit_sm_resp.command_status = 0x00. Not sure, but is seems to me that this specific SMCS does not return a message_id *IF* the ESME hasn't requested a delivery report, and if he has requested then they provide the message_id. It still mis-interprets the spec, but at least we can handle it in the sense that the daemon doesn't PANIC out and keeps on running. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Sorry, it doesn't mean setting default value during table creation right? Because this approach will still might be interupted by user's value. Thanks. On 17 Sep 2013 21:57, Stipe Tolj st...@kannel.org wrote: Am 17.09.2013 16:54, schrieb Willy Mularto: Thanks for your review. The other approach is to override the sms_type value during the INSERT. yep, most DBMSs allow setting default values, in case the INSERT statement does not insert a specific value for the field. That was my idea. It's cleaner, and leaves the value injection to the DB layer. Stipe -- --**--**--- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --**--**---
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 16:54, schrieb Willy Mularto: Thanks for your review. The other approach is to override the sms_type value during the INSERT. yep, most DBMSs allow setting default values, in case the INSERT statement does not insert a specific value for the field. That was my idea. It's cleaner, and leaves the value injection to the DB layer. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Thanks for your review. The other approach is to override the sms_type value during the INSERT. On 17 Sep 2013 21:37, Stipe Tolj st...@kannel.org wrote: Am 17.09.2013 03:06, schrieb Willy Mularto: +1 It will be good to dump it to bearerbox-access log and flag it as FAILED MT and maybe add more info into it so we can analyse it. Thanks Stipe. BTW, committed to svn trunk. Disagree here for the FAILED SMS access-log entry, since it is logically not failing. The SMSC *HAS* accepted the submit_sm PDU, since we get a submit_sm_resp.command_status = 0x00. Not sure, but is seems to me that this specific SMCS does not return a message_id *IF* the ESME hasn't requested a delivery report, and if he has requested then they provide the message_id. It still mis-interprets the spec, but at least we can handle it in the sense that the daemon doesn't PANIC out and keeps on running. Stipe -- --**--**--- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --**--**---
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 17:22, schrieb Willy Mularto: Sorry, it doesn't mean setting default value during table creation right? Because this approach will still might be interupted by user's value. Thanks. The default value is defined during table creation, yes. The point is: IF the user doesn't insert a specific value for the field THEN the default is applied. If the user specifically inserts -1 then he will get -1. But that makes no logical sense. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
MO tables are not impacted by the problem of having an empty column, are they? If that's the case I'd leave them with no default. On Tue, Sep 17, 2013 at 3:47 PM, Stipe Tolj st...@kannel.org wrote: Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --**--**--- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --**--**---
Re: [PATCH] No PANIC on failing store_load()
I think first edit @ 20th line is not needed. @@ -7,7 +7,7 @@ momt ENUM('MO', 'MT', 'DLR') NULL, sender VARCHAR(20) NULL, \ receiver VARCHAR(20) NULL, udhdata BLOB NULL, msgdata TEXT NULL, \ time BIGINT(20) NULL, smsc_id VARCHAR(255) NULL, service VARCHAR(255) NULL, \ -account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NULL, \ +account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NOT NULL DEFAULT 2, \ mclass BIGINT(20) NULL, mwi BIGINT(20) NULL, coding BIGINT(20) NULL, \ compress BIGINT(20) NULL, validity BIGINT(20) NULL, deferred BIGINT(20) NULL, \ dlr_mask BIGINT(20) NULL, dlr_url VARCHAR(255) NULL, pid BIGINT(20) NULL, \ because it edits definition of sent_sms table, but you need only send_sms, because records inserted there (which is a 2nd edit - #define SQLBOX_MYSQL_CREATE_INSERT_TABLE ...) 2013/9/17 Stipe Tolj st...@kannel.org: Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
2013/9/18 spameden spame...@gmail.com: I think first edit @ 20th line is not needed. @@ -7,7 +7,7 @@ momt ENUM('MO', 'MT', 'DLR') NULL, sender VARCHAR(20) NULL, \ receiver VARCHAR(20) NULL, udhdata BLOB NULL, msgdata TEXT NULL, \ time BIGINT(20) NULL, smsc_id VARCHAR(255) NULL, service VARCHAR(255) NULL, \ -account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NULL, \ +account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NOT NULL DEFAULT 2, \ mclass BIGINT(20) NULL, mwi BIGINT(20) NULL, coding BIGINT(20) NULL, \ compress BIGINT(20) NULL, validity BIGINT(20) NULL, deferred BIGINT(20) NULL, \ dlr_mask BIGINT(20) NULL, dlr_url VARCHAR(255) NULL, pid BIGINT(20) NULL, \ because it edits definition of sent_sms table, but you need only send_sms, because records inserted there (which is a 2nd edit - #define SQLBOX_MYSQL_CREATE_INSERT_TABLE ...) I'm sorry in first sentence I've said first edit @ 20 is not needed, but actually I meant first edit @ 7th line. So the correct patch would look like (for only send_sms table): --- sqlbox_mysql.h2013-09-18 00:01:37.0 +0400 +++ sqlbox_mysql.h.edit2013-09-18 00:02:30.0 +0400 @@ -20,7 +20,7 @@ momt ENUM('MO', 'MT') NULL, sender VARCHAR(20) NULL, \ receiver VARCHAR(20) NULL, udhdata BLOB NULL, msgdata TEXT NULL, \ time BIGINT(20) NULL, smsc_id VARCHAR(255) NULL, service VARCHAR(255) NULL, \ -account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NULL, \ +account VARCHAR(255) NULL, id BIGINT(20) NULL, sms_type BIGINT(20) NULL DEFAULT 2, \ mclass BIGINT(20) NULL, mwi BIGINT(20) NULL, coding BIGINT(20) NULL, \ compress BIGINT(20) NULL, validity BIGINT(20) NULL, deferred BIGINT(20) NULL, \ dlr_mask BIGINT(20) NULL, dlr_url VARCHAR(255) NULL, pid BIGINT(20) NULL, \ 2013/9/17 Stipe Tolj st...@kannel.org: Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
sms_type is MT direction only AFAIK. On Sep 18, 2013, at 2:49 AM, Stipe Tolj st...@kannel.org wrote: Am 17.09.2013 18:56, schrieb Rene Kluwen: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. +1 from me. What about the direction implication? Is this table only for the MT direction? For an MO table we may have an other value for sms_type here. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
Hi Rene, Many thanks for the explanation. But for me, setting 2 as default and or fixed value during INSERT is the cleanest way and surely will avoid the panic condition rather than during the table creation. A little patch on sqlbox_mysql.c and well this is only my personal approach. On Sep 17, 2013, at 11:56 PM, Rene Kluwen rene.klu...@chimit.nl wrote: Agreed with Stipe here. If the users enters a different value for sms_type, it is their responsibility. What is covered here, is an accidental wrong value for sms_type, because nowhere is documented that sms_type should be set to MT (value '2') because otherwise Kannel will panic. Attached is a simple solution that will benefit everyone. Which is ready to be committed. == Rene -Original Message- From: devel [mailto:devel-boun...@kannel.org] On Behalf Of Stipe Tolj Sent: dinsdag 17 september 2013 17:55 Cc: kannel_dev_mailinglist devel@kannel. org Subject: Re: [PATCH] No PANIC on failing store_load() Am 17.09.2013 17:22, schrieb Willy Mularto: Sorry, it doesn't mean setting default value during table creation right? Because this approach will still might be interupted by user's value. Thanks. The default value is defined during table creation, yes. The point is: IF the user doesn't insert a specific value for the field THEN the default is applied. If the user specifically inserts -1 then he will get -1. But that makes no logical sense. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- default.diff
RE: [PATCH] No PANIC on failing store_load()
Hi, This sounds like a good change. This principle should be applied, in general. Regards, Kelvin R. Porter -Original Message- From: devel [mailto:devel-boun...@kannel.org] On Behalf Of Stipe Tolj Sent: Monday, September 16, 2013 10:59 AM To: devel@kannel.org Subject: [PATCH] No PANIC on failing store_load() Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
[PATCH] No PANIC on failing store_load()
Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- Index: gw/bearerbox.c === --- gw/bearerbox.c (revision 5048) +++ gw/bearerbox.c (working copy) @@ -658,6 +658,8 @@ static void dispatch_into_queue(Msg *msg) { +char id[UUID_STR_LEN + 1]; + gw_assert(msg != NULL), gw_assert(msg_type(msg) == sms); @@ -672,7 +674,11 @@ gwlist_append(incoming_sms, msg); break; default: -panic(0, Not handled sms_type within store!); +uuid_unparse(msg-sms.id, id); +error(0, Not handled sms_type %ld within store for message ID %s, + msg-sms.sms_type, id); +msg_destroy(msg); +break; } }
Re: [PATCH] No PANIC on failing store_load()
+1 for this patch! I remember struggling with sqlbox without injecting proper sms_type = 2 and after restart kannel was crashing. I had to manually clean specific message out of spool. 2013/9/16 Porter, Kelvin kelvin.por...@h3net.com: Hi, This sounds like a good change. This principle should be applied, in general. Regards, Kelvin R. Porter -Original Message- From: devel [mailto:devel-boun...@kannel.org] On Behalf Of Stipe Tolj Sent: Monday, September 16, 2013 10:59 AM To: devel@kannel.org Subject: [PATCH] No PANIC on failing store_load() Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org ---
Re: [PATCH] No PANIC on failing store_load()
+1 this was really an annoyance IMHO -- Alejandro Guerrieri On Sep 16, 2013, at 11:59 AM, Stipe Tolj st...@kannel.org wrote: Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- store_load_no_panic.diff
Re: [PATCH] No PANIC on failing store_load()
+1 It will be good to dump it to bearerbox-access log and flag it as FAILED MT and maybe add more info into it so we can analyse it. Thanks Stipe. On Sep 16, 2013, at 11:56 PM, Alejandro Guerrieri alejandro.guerri...@gmail.com wrote: +1 this was really an annoyance IMHO -- Alejandro Guerrieri On Sep 16, 2013, at 11:59 AM, Stipe Tolj st...@kannel.org wrote: Hi list, attached is a very simple change in behavior in being more runtime constraining, in case a message in the spool has an invalid 'sms_type' we SHOULD NOT PANIC i.e. but rather dump an ERROR entry and try to continue. Otherwise we may enter an infinite PANIC/restart loop for cases that such messages get somehow injected to the spool. If no objections, will be committing soon. Stipe -- --- Kölner Landstrasse 419 40589 Düsseldorf, NRW, Germany tolj.org system architecture Kannel Software Foundation (KSF) http://www.tolj.org/ http://www.kannel.org/ mailto:st_{at}_tolj.org mailto:stolj_{at}_kannel.org --- store_load_no_panic.diff