Re: [PATCH] BUG on DLR parsing

Hi,

some comments to your patch:

+        	/*
+        	 * 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)));

here you overwrite new values with old values. Try dict_put_once  
instead.

+        		}
+        		gwlist_destroy(keys, NULL);
+        	}
+			dict_destroy(curr->values);
+			curr->values = (Dict*)dict;
		^^^^^^
		indentation

+    struct timeval last_mt_microtime;  /* the last microtime a MT was  
sent over the SMSC */
+} SMPP;
+

seems some throttling part is there. Please drop it.

+    /* 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;

ditto

Please only one logical change in patch.

Thanks,
Alex

Am 30.04.2009 um 23:14 schrieb Alejandro Guerrieri:

> 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]
>
> <kannel-dlr-meta-data.patch>
>
> 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>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>