I've removed the throttling patch, fixed the indent and the dict_put_once.
Please see attached. Regards, -- Alejandro Guerrieri aguerri...@kannel.org
kannel-dlr-meta-data-20090504.patch
Description: Binary data
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 aguerri...@kannel.org <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 aguerri...@kannel.org 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 aguerri...@kannel.org 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 aguerri...@kannel.org <kannel-smsc-dlr-err.patch>
