Re: [PATCH] BUG on DLR parsing

ok, commited to cvs.

Thanks,
Alex

Am 04.05.2009 um 13:11 schrieb Alejandro Guerrieri:

> You're right, part of a throttling patch slept in. This wasn't  
> intended, sorry :P
>
> I've removed the throttling  patch,  fixed the indent and the  
> dict_put_once.
>
> Please see attached.
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
> <kannel-dlr-meta-data-20090504.patch>
> On 04/05/2009, at 11:22, Alexander Malysh wrote:
>
>> 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.
>>
>
> True.
>
>> +        		}
>> +        		gwlist_destroy(keys, NULL);
>> +        	}
>> +			dict_destroy(curr->values);
>> +			curr->values = (Dict*)dict;
>> 		^^^^^^
>> 		indentation
>
> Fixed
>
>> +    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.
>>
>
> Yes, this was unintended, sorry.
>
>> +    /* 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>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>