Re: [PATCH] BUG on DLR parsing

Alejandro Guerrieri <[email protected]>
Newsgroups gmane.comp.mobile.kannel.devel
Message-ID <[email protected]> 
Ok, please have a look at the attached patch. I had to modify  
meta_data_set_values (plural) to allow for appending (otherwise it  
would erase the data coming from the dlr with the data from the pdu).  
I think it's a nice feature to have anyway, just like  
meta_data_set_value (singular) does.

Regards,
--
Alejandro Guerrieri
[email protected]



On 30/04/2009, at 15:32, Alexander Malysh wrote:

>
> Am 30.04.2009 um 15:20 schrieb Alejandro Guerrieri:
>
>> Ok, I definitely could do that, the only problem is how will I test  
>> for it afterwards (not sure if my test binds work with that tlv).
>>
>> I'll figure out something I guess ;)
>
> modify test/drive_smpp.c ;)
>
>>
>> Regards,
>> --
>> Alejandro Guerrieri
>> [email protected]
>>
>>
>>
>> On 30/04/2009, at 14:18, Alexander Malysh wrote:
>>
>>> Hi,
>>>
>>> +1 for this patch :)
>>>
>>> As to the original patch, you should first check for  
>>> network_error_code TLV before going to parse
>>> message payload. Only of it's not there you should parse payload.  
>>> So in this form -1 for this patch.
>>>
>>> Thanks,
>>> Alex
>>>
>>> P.S. Please integrate both patches together.
>>>
>>> Am 30.04.2009 um 12:46 schrieb Alejandro Guerrieri:
>>>
>>>> Alex,
>>>>
>>>> What about something like this (in addition to my former patch):
>>>>
>>>> Index: gw/smsc/smsc_smpp.c
>>>> ===================================================================
>>>> --- gw/smsc/smsc_smpp.c	(revision 29)
>>>> +++ gw/smsc/smsc_smpp.c	(working copy)
>>>> @@ -1374,11 +1374,16 @@
>>>>       * we found the delivery report in our storage, so recode the
>>>>       * message structure.
>>>>       * The DLR trigger URL is indicated by msg->sms.dlr_url.
>>>> -         * Add the DLR error code as billing identifier.
>>>> +         * Add the DLR error code to meta-data.
>>>>       */
>>>>      dlrmsg->sms.msgdata = octstr_duplicate(respstr);
>>>>      dlrmsg->sms.sms_type = report_mo;
>>>> -        dlrmsg->sms.binfo = octstr_duplicate(err);
>>>> +        if (err != NULL) {
>>>> +            if (dlrmsg->sms.meta_data == NULL) {
>>>> +            	dlrmsg->sms.meta_data = octstr_create("");
>>>> +            }
>>>> +        	meta_data_set_value(dlrmsg->sms.meta_data, "smpp",  
>>>> octstr_imm("dlr_err"), err, 1);
>>>> +        }
>>>>  } else {
>>>>      error(0,"SMPP[%s]: got DLR but could not find message or was  
>>>> not interested "
>>>>              "in it id<%s> dst<%s>, type<%d>",
>>>>
>>>> Regards,
>>>> --
>>>> Alejandro Guerrieri
>>>> [email protected]
>>>>
>>>>
>>>>
>>>> On 30/04/2009, at 9:30, Alexander Malysh wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> I don't like passing err in binfo field. IMO binfo should be  
>>>>> used for billing identifier but not for smpp error code.
>>>>> I would prefer to see patch that drop err from binfo and make  
>>>>> use of meta-data (group dlr?).
>>>>>
>>>>> Thanks,
>>>>> Alex
>>>>>
>>>>> Am 29.04.2009 um 18:10 schrieb Alejandro Guerrieri:
>>>>>
>>>>>> This patch fixes a bug when parsing DLR's:
>>>>>>
>>>>>> As the code is now, if a DLR having a receipted_message_id, the  
>>>>>> DLR text is not parsed, so the "err" and "stat" fields are empty.
>>>>>>
>>>>>> In particular, the "err" code is being passed on the "binfo"  
>>>>>> field, so this remained empty if a receipted_message_id is  
>>>>>> present (because the sscanf code was not executed).
>>>>>>
>>>>>> Attached patch fixes that part, so the "err" parameter is  
>>>>>> present on the binfo field on all cases.
>>>>>>
>>>>>> Regards,
>>>>>> --
>>>>>> Alejandro Guerrieri
>>>>>> [email protected]
>>>>>>
>>>>>>
>>>>>> <kannel-smsc-dlr-err.patch>
>>>>>>
>>>>>
>>>>
>>>
>>
>
kannel-dlr-meta-data.patch (application/octet-stream, 15.4 KB) 
Index: gw/meta_data.h
===================================================================
--- gw/meta_data.h	(revision 27)
+++ gw/meta_data.h	(working copy)
@@ -72,7 +72,7 @@
 /**
  * Replace Dictionary for the given group.
  */
-int meta_data_set_values(Octstr *data, const Dict *dict, const char *group);
+int meta_data_set_values(Octstr *data, const Dict *dict, const char *group, int replace);
 /**
  * Set or replace value for a given group and key.
  */
Index: gw/meta_data.c
===================================================================
--- gw/meta_data.c	(revision 27)
+++ gw/meta_data.c	(working copy)
@@ -270,10 +270,12 @@
 }
 
 
-int meta_data_set_values(Octstr *data, const Dict *dict, const char *group)
+int meta_data_set_values(Octstr *data, const Dict *dict, const char *group, int replace)
 {
     struct meta_data *mdata, *curr;
     int i;
+    List *keys;
+    Octstr *key;
 
     if (data == NULL || group == NULL)
         return -1;
@@ -281,8 +283,18 @@
     mdata = meta_data_unpack(data);
     for (curr = mdata; curr != NULL; curr = curr->next) {
         if (octstr_str_case_compare(curr->group, group) == 0) {
-            dict_destroy(curr->values);
-            curr->values = (Dict*)dict;
+        	/*
+        	 * If we don't replace the values, copy the old Dict values to the new Dict
+        	 */
+        	if (replace == 0) {
+        		keys = dict_keys(curr->values);
+        		while((key = gwlist_extract_first(keys)) != NULL) {
+        			dict_put((Dict*)dict, key, octstr_duplicate(dict_get(curr->values, key)));
+        		}
+        		gwlist_destroy(keys, NULL);
+        	}
+			dict_destroy(curr->values);
+			curr->values = (Dict*)dict;
             break;
         }
     }
Index: gw/smsc/smsc_smpp.c
===================================================================
--- gw/smsc/smsc_smpp.c	2 Apr 2009 20:30:19 -0000	1.114
+++ gw/smsc/smsc_smpp.c	30 Apr 2009 20:51:04 -0000
@@ -166,10 +166,10 @@
     long connection_timeout;
     long wait_ack;
     int wait_ack_action;
-    SMSCConn *conn;
-} SMPP;
-
-
+    SMSCConn *conn; 
+    struct timeval last_mt_microtime;  /* the last microtime a MT was sent over the SMSC */
+} SMPP; 
+ 
 struct smpp_msg {
     time_t sent_time;
     Msg *msg;
@@ -463,6 +463,10 @@
     msg->sms.binfo = pdu->u.deliver_sm.service_type;
     pdu->u.deliver_sm.service_type = NULL;
 
+    /* Foreign ID on MO's Patch */
+    msg->sms.foreign_id = pdu->u.deliver_sm.receipted_message_id;
+    pdu->u.deliver_sm.receipted_message_id = NULL;
+
     if (pdu->u.deliver_sm.esm_class & ESM_CLASS_SUBMIT_RPI)
         msg->sms.rpi = 1;
 
@@ -562,7 +566,7 @@
 
     if (msg->sms.meta_data == NULL)
         msg->sms.meta_data = octstr_create("");
-    meta_data_set_values(msg->sms.meta_data, pdu->u.deliver_sm.tlv, "smpp");
+    meta_data_set_values(msg->sms.meta_data, pdu->u.deliver_sm.tlv, "smpp", 1);
 
     return msg;
 
@@ -627,6 +631,10 @@
     msg->sms.binfo = pdu->u.data_sm.service_type;
     pdu->u.data_sm.service_type = NULL;
 
+    /* Foreign ID on MO's Patch */
+    msg->sms.foreign_id = pdu->u.data_sm.receipted_message_id;
+    pdu->u.data_sm.receipted_message_id = NULL;
+    
     if (pdu->u.data_sm.esm_class & ESM_CLASS_SUBMIT_RPI)
         msg->sms.rpi = 1;
 
@@ -712,7 +720,7 @@
 
     if (msg->sms.meta_data == NULL)
         msg->sms.meta_data = octstr_create("");
-    meta_data_set_values(msg->sms.meta_data, pdu->u.data_sm.tlv, "smpp");
+    meta_data_set_values(msg->sms.meta_data, pdu->u.data_sm.tlv, "smpp", 1);
 
     return msg;
 
@@ -1002,6 +1010,9 @@
     SMPP_PDU *pdu;
     Octstr *os;
     double delay = 0;
+    struct timeval tv;
+    double mt_microtime = 0;
+    double microtime = 0;
 
     if (*pending_submits == -1)
         return;
@@ -1033,9 +1044,35 @@
             /*
              * obey throughput speed limit, if any.
              */
+            mt_microtime = (double) smpp->last_mt_microtime.tv_sec + (double) smpp->last_mt_microtime.tv_usec / 1000000;
+
+            debug("bb.sms.smpp", 0, "SMPP[%s]: QOS: last_mt_microtime:<%f> now:<%f>",
+            octstr_get_cstr(smpp->conn->id), mt_microtime, microtime);
+
             if (smpp->conn->throughput > 0)
-                gwthread_sleep(delay);
-        } else { /* write error occurs */
+              {
+                debug("bb.sms.smpp", 0, "SMPP[%s]: QOS: Sleeping <%f>sec", octstr_get_cstr(smpp->conn->id), delay);
+                gwthread_sleep(delay); 
+
+                gettimeofday(&tv, 0);
+                microtime = (double) tv.tv_sec + (double) tv.tv_usec / 1000000;
+
+                debug("bb.sms.smpp", 0, "SMPP[%s]: QOS: Sleeping Done.", octstr_get_cstr(smpp->conn->id));
+                while (mt_microtime + delay > microtime)
+                  {
+                    debug("bb.sms.smpp", 0, "SMPP[%s]: QOS: Traffic Policy exceeded, we need to sleep <%f>sec",
+                      octstr_get_cstr(smpp->conn->id), (mt_microtime + delay) - microtime);
+                    /* Sleeping rest of time */
+                    gwthread_sleep((mt_microtime + delay) - microtime);
+
+                    /* Refreshing time */
+                    gettimeofday(&tv, 0);
+                    microtime = (double) tv.tv_sec + (double) tv.tv_usec / 1000000;
+                  }
+                gettimeofday(&smpp->last_mt_microtime, 0);
+              }
+        }
+        else { /* write error occurs */
             smpp_pdu_destroy(pdu);
             bb_smscconn_send_failed(smpp->conn, msg, SMSCCONN_FAILED_TEMPORARILY, NULL);
             break;
@@ -1171,7 +1208,7 @@
 }
 
 
-static Msg *handle_dlr(SMPP *smpp, Octstr *destination_addr, Octstr *short_message, Octstr *message_payload, Octstr *receipted_message_id, long message_state)
+static Msg *handle_dlr(SMPP *smpp, Octstr *destination_addr, Octstr *short_message, Octstr *message_payload, Octstr *receipted_message_id, long message_state, Octstr *network_error_code)
 {
     Msg *dlrmsg = NULL;
     Octstr *respstr = NULL, *msgid = NULL, *err = NULL, *tmp;
@@ -1208,89 +1245,95 @@
         }
     }
 
-    /* check for SMPP v.3.4. and message_payload */
-    if (smpp->version > 0x33 && octstr_len(short_message) == 0)
-        respstr = message_payload;
-    else
-        respstr = short_message;
-
-    /* still no msgid or dlrstat ? */
-    if ((msgid == NULL || dlrstat == -1) && respstr) {
-        long curr = 0, vpos = 0;
-        Octstr *stat = NULL;
-        char id_cstr[65], stat_cstr[16], sub_d_cstr[13], done_d_cstr[13];
-        char err_cstr[4];
-        int sub, dlrvrd, ret;
-
-        /* get server message id */
-        /* first try sscanf way if thus failed then old way */
-        ret = sscanf(octstr_get_cstr(respstr),
-                     "id:%64[^s] sub:%d dlvrd:%d submit date:%12[0-9] done "
-                     "date:%12[0-9] stat:%10[^t^e] err:%3[0-9]",
-                     id_cstr, &sub, &dlrvrd, sub_d_cstr, done_d_cstr,
-                     stat_cstr, err_cstr);
-        if (ret == 7) {
-            /* only if not already here */
-            if (msgid == NULL) {
-                msgid = octstr_create(id_cstr);
-                octstr_strip_blanks(msgid);
-            }
-            stat = octstr_create(stat_cstr);
-            octstr_strip_blanks(stat);
-            err = octstr_create(err_cstr);
-            octstr_strip_blanks(err);
-        }
-        else {
-            debug("bb.sms.smpp", 0, "SMPP[%s]: Couldnot parse DLR string sscanf way,"
-                "fallback to old way. Please report!", octstr_get_cstr(smpp->conn->id));
-
-            /* only if not already here */
-            if (msgid == NULL) {
-                if ((curr = octstr_search(respstr, octstr_imm("id:"), 0)) != -1) {
-                    vpos = octstr_search_char(respstr, ' ', curr);
-                    if ((vpos-curr >0) && (vpos != -1))
-                        msgid = octstr_copy(respstr, curr+3, vpos-curr-3);
-                } else {
-                    msgid = NULL;
-                }
-            }
-
-            /* get err & status code */
-            if ((curr = octstr_search(respstr, octstr_imm("stat:"), 0)) != -1) {
-                vpos = octstr_search_char(respstr, ' ', curr);
-                if ((vpos-curr >0) && (vpos != -1))
-                    stat = octstr_copy(respstr, curr+5, vpos-curr-5);
-            } else {
-                stat = NULL;
-            }
-            if ((curr = octstr_search(respstr, octstr_imm("err:"), 0)) != -1) {
-                vpos = octstr_search_char(respstr, ' ', curr);
-                if ((vpos-curr >0) && (vpos != -1))
-                    err = octstr_copy(respstr, curr+4, vpos-curr-4);
-            } else {
-                err = NULL;
-            }
-        }
-
-        /*
-         * we get the following status:
-         * DELIVRD, ACCEPTD, EXPIRED, DELETED, UNDELIV, UNKNOWN, REJECTD
-         *
-         * Note: some buggy SMSC's send us immediately delivery notifications although
-         *          we doesn't requested these.
-         */
-        if (stat != NULL && octstr_compare(stat, octstr_imm("DELIVRD")) == 0)
-            dlrstat = DLR_SUCCESS;
-        else if (stat != NULL && (octstr_compare(stat, octstr_imm("ACCEPTD")) == 0 ||
-                        octstr_compare(stat, octstr_imm("ACKED")) == 0 ||
-                        octstr_compare(stat, octstr_imm("BUFFRED")) == 0 ||
-                        octstr_compare(stat, octstr_imm("BUFFERD")) == 0 ||
-                        octstr_compare(stat, octstr_imm("ENROUTE")) == 0))
-            dlrstat = DLR_BUFFERED;
-        else
-            dlrstat = DLR_FAIL;
+    if (network_error_code != NULL)
+    	err = octstr_duplicate(network_error_code);
 
-        octstr_destroy(stat);
+    /* check for SMPP v.3.4. and message_payload */
+	if (smpp->version > 0x33 && octstr_len(short_message) == 0)
+		respstr = message_payload;
+	else
+		respstr = short_message;
+    
+    if (err == NULL || dlrstat == -1) {
+		/* parse the respstr if it exists */
+		if (respstr) {
+			long curr = 0, vpos = 0;
+			Octstr *stat = NULL;
+			char id_cstr[65], stat_cstr[16], sub_d_cstr[13], done_d_cstr[13];
+			char err_cstr[4];
+			int sub, dlrvrd, ret;
+	
+			/* get server message id */
+			/* first try sscanf way if thus failed then old way */
+			ret = sscanf(octstr_get_cstr(respstr),
+						 "id:%64[^s] sub:%d dlvrd:%d submit date:%12[0-9] done "
+						 "date:%12[0-9] stat:%10[^t^e] err:%3[0-9]",
+						 id_cstr, &sub, &dlrvrd, sub_d_cstr, done_d_cstr,
+						 stat_cstr, err_cstr);
+			if (ret == 7) {
+				/* only if not already here */
+				if (msgid == NULL) {
+					msgid = octstr_create(id_cstr);
+					octstr_strip_blanks(msgid);
+				}
+				stat = octstr_create(stat_cstr);
+				octstr_strip_blanks(stat);
+				err = octstr_create(err_cstr);
+				octstr_strip_blanks(err);
+			}
+			else {
+				debug("bb.sms.smpp", 0, "SMPP[%s]: Couldnot parse DLR string sscanf way,"
+					"fallback to old way. Please report!", octstr_get_cstr(smpp->conn->id));
+	
+				/* only if not already here */
+				if (msgid == NULL) {
+					if ((curr = octstr_search(respstr, octstr_imm("id:"), 0)) != -1) {
+						vpos = octstr_search_char(respstr, ' ', curr);
+						if ((vpos-curr >0) && (vpos != -1))
+							msgid = octstr_copy(respstr, curr+3, vpos-curr-3);
+					} else {
+						msgid = NULL;
+					}
+				}
+	
+				/* get err & status code */
+				if ((curr = octstr_search(respstr, octstr_imm("stat:"), 0)) != -1) {
+					vpos = octstr_search_char(respstr, ' ', curr);
+					if ((vpos-curr >0) && (vpos != -1))
+						stat = octstr_copy(respstr, curr+5, vpos-curr-5);
+				} else {
+					stat = NULL;
+				}
+				if ((curr = octstr_search(respstr, octstr_imm("err:"), 0)) != -1) {
+					vpos = octstr_search_char(respstr, ' ', curr);
+					if ((vpos-curr >0) && (vpos != -1))
+						err = octstr_copy(respstr, curr+4, vpos-curr-4);
+				} else {
+					err = NULL;
+				}
+			}
+	
+			/*
+			 * we get the following status:
+			 * DELIVRD, ACCEPTD, EXPIRED, DELETED, UNDELIV, UNKNOWN, REJECTD
+			 *
+			 * Note: some buggy SMSC's send us immediately delivery notifications although
+			 *          we doesn't requested these.
+			 */
+			if (dlrstat == -1) {
+				if (stat != NULL && octstr_compare(stat, octstr_imm("DELIVRD")) == 0)
+					dlrstat = DLR_SUCCESS;
+				else if (stat != NULL && (octstr_compare(stat, octstr_imm("ACCEPTD")) == 0 ||
+								octstr_compare(stat, octstr_imm("ACKED")) == 0 ||
+								octstr_compare(stat, octstr_imm("BUFFRED")) == 0 ||
+								octstr_compare(stat, octstr_imm("BUFFERD")) == 0 ||
+								octstr_compare(stat, octstr_imm("ENROUTE")) == 0))
+					dlrstat = DLR_BUFFERED;
+				else
+					dlrstat = DLR_FAIL;
+			}
+			octstr_destroy(stat);
+		}
     }
 
     if (msgid != NULL && dlrstat != -1) {
@@ -1336,11 +1379,16 @@
          * we found the delivery report in our storage, so recode the
          * message structure.
          * The DLR trigger URL is indicated by msg->sms.dlr_url.
-         * Add the DLR error code as billing identifier.
+         * Add the DLR error code to meta-data.
          */
         dlrmsg->sms.msgdata = octstr_duplicate(respstr);
         dlrmsg->sms.sms_type = report_mo;
-        dlrmsg->sms.binfo = octstr_duplicate(err);
+        if (err != NULL) {
+            if (dlrmsg->sms.meta_data == NULL) {
+            	dlrmsg->sms.meta_data = octstr_create("");
+            }
+        	meta_data_set_value(dlrmsg->sms.meta_data, "smpp", octstr_imm("dlr_err"), err, 1);
+        }
     } else {
         error(0,"SMPP[%s]: got DLR but could not find message or was not interested "
                 "in it id<%s> dst<%s>, type<%d>",
@@ -1403,11 +1451,11 @@
                 debug("bb.sms.smpp",0,"SMPP[%s] handle_pdu, got DLR",
                       octstr_get_cstr(smpp->conn->id));
                 dlrmsg = handle_dlr(smpp, pdu->u.data_sm.source_addr, NULL, pdu->u.data_sm.message_payload,
-                                    pdu->u.data_sm.receipted_message_id, pdu->u.data_sm.message_state);
+                                    pdu->u.data_sm.receipted_message_id, pdu->u.data_sm.message_state, pdu->u.data_sm.network_error_code);
                 if (dlrmsg != NULL) {
                     if (dlrmsg->sms.meta_data == NULL)
                         dlrmsg->sms.meta_data = octstr_create("");
-                    meta_data_set_values(dlrmsg->sms.meta_data, pdu->u.data_sm.tlv, "smpp");
+                    meta_data_set_values(dlrmsg->sms.meta_data, pdu->u.data_sm.tlv, "smpp", 0);
                     /* passing DLR to upper layer */
                     reason = bb_smscconn_receive(smpp->conn, dlrmsg);
                 } else {
@@ -1461,12 +1509,12 @@
                       octstr_get_cstr(smpp->conn->id));
 
                 dlrmsg = handle_dlr(smpp, pdu->u.deliver_sm.source_addr, pdu->u.deliver_sm.short_message, pdu->u.deliver_sm.message_payload,
-                                    pdu->u.deliver_sm.receipted_message_id, pdu->u.deliver_sm.message_state);
+                                    pdu->u.deliver_sm.receipted_message_id, pdu->u.deliver_sm.message_state, pdu->u.deliver_sm.network_error_code);
                 resp = smpp_pdu_create(deliver_sm_resp, pdu->u.deliver_sm.sequence_number);
                 if (dlrmsg != NULL) {
                     if (dlrmsg->sms.meta_data == NULL)
                         dlrmsg->sms.meta_data = octstr_create("");
-                    meta_data_set_values(dlrmsg->sms.meta_data, pdu->u.deliver_sm.tlv, "smpp");
+                    meta_data_set_values(dlrmsg->sms.meta_data, pdu->u.deliver_sm.tlv, "smpp", 0);
                     reason = bb_smscconn_receive(smpp->conn, dlrmsg);
                 } else
                     reason = SMSCCONN_SUCCESS;
@@ -2251,4 +2299,3 @@
 
     return 0;
 }
-
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.