Re: [PATCH] Meta Data on DLR's

Sun, 06 Dec 2009 11:59:35 -0800

We added this because some users needed to get command_status values. See Dante Moreno's message on Aug 30 to user's list:


I'm attaching the thread from devel list as well. You actually agreed to it then and suggested using the 0x%08lx format for it.

--- Begin Message ---
Ok, silly diff errors aside (I wasn't including dlr_p.h :P), here's the new patches also changing the error code format as Alex suggested.

Attachment: kannel-dlr-meta-data.diff
Description: Binary data

Attachment: kannel-dlr-command-status.diff
Description: Binary data


Userguide patch coming later.

Just a few more ideas to consider:

1. Adding one more called "dlr_status_text" with the text description from smpp_error_to_string(pdu->u.submit_sm_resp.command_status). Not sure if it makes sense, this could be done at the application level with a simple table...

2. Adding one more called "dlr_sequence" with the sequence_number parameter.

3. Extending patch #1 adding the meta-data column on all DB implementations, so meta-data could be passed and stored on dlr's other than the first one generated by kannel.

Regards,
--
Alejandro Guerrieri



On 03/09/2009, at 23:33, Alexander Malysh wrote:

Hi Alex,

first patch seems still broken:
+    dlr->meta_data = (msg->sms.meta_data ? octstr_duplicate(msg->sms.meta_data) : octstr_create(""));

we don't have meta_data in dlr struct. otherwise +1

second patch:
+            if (msg->sms.meta_data == NULL)
+                msg->sms.meta_data = octstr_create("");
+            meta_data_set_value(msg->sms.meta_data, "smpp", octstr_imm("dlr_status"),
+                                octstr_format("%d", pdu->u.submit_sm_resp.command_status), 1);
+

Would it it not better to have error code formated as smpp-tlv group and as error message?
+                                octstr_format("0x%08lx", pdu->u.submit_sm_resp.command_status), 1);

Thanks,
Alexander Malysh


Am 03.09.2009 um 21:13 schrieb Alejandro Guerrieri:

Oh, I see, I've attached the ~ backup file from vim. This are the right files, just in case:

<kannel-dlr-command-status.diff>
<kannel-dlr-meta-data.diff>

--
Alejandro Guerrieri



On 03/09/2009, at 21:06, Alejandro Guerrieri wrote:

Oh, that must have filtered into the patch. Weird :P

Sorry, it's safe to remove it.

Regards,
--
Alejandro Guerrieri



On 03/09/2009, at 20:56, Gustavo Mohme C. wrote:

Hi Alejandro,
I get this error after applying your patch.

gw/dlr.c: In function ‘dlr_add’:
gw/dlr.c:352: error: ‘struct dlr_entry’ has no member named ‘meta_data’
make: *** [gw/dlr.o] Error 1

After editing /gw/dlr_p.h and adding meta_data to the struct dlr_entry, it compiled with no complaints. Is this the correct thing to do?
I was patching the latest kannel snapshot....
Thanks,
Gustavo

On Thu, Sep 3, 2009 at 3:39 AM, Alejandro Guerrieri <[email protected]> wrote:
Hi,

This is a set of two patches, though they're both very simple and could have been mixed, I've preferred to split to honor the rule of not mixing features in a single patch.

Patch #1, kannel-drl-meta-data.diff allows meta-data to be passed when sending a message to come back into the "fake" dlr that kannel generates when the smsc accepts or rejects a message. It could be extended to work with the different dlr engines so the meta-data would be also passed on the intermediate and final dlr's delivered by the carriers (though it would require changes on the database structure to add a "meta_data" column of course. I'm not sure this would be needed, the dlr-url could be easily abused to pass application-specific parameters without much hassle.

Patch #2, kannel-dlr-command-status.diff builds on top of patch #1 and creates a meta data value called "dlr_status" that comes back on the "fake" dlr generated by kannel (This was the real reason why patch #1 was created). The value there is the "command_status" value some people on the list needs to get from the submit_sm_resp PDU's.

Example usage:

On sendsms:

http://localhost:13013/cgi-bin/sendsms?username=kannel&password=kannel&from=12345&to=12345678&smsc=mysmsc&text=Hello&dlr-mask=31&meta-data="">

Notes:

meta-data is urlencoded version of: ?smpp?my_own_field=1234
dlr-url is urlencoded version of: http://localhost/x?data="">

So, after applying Patch #1, kannel would call the following url:

http://localhost/x?md=%3Fsmpp%3Fmy_own_field%3D1234

The "md" parameter, once urldecoded would look like:

?smpp?my_own_field=1234

You can pass as many parameters as you want of course and they would be added to meta-data along with any other fields you've defined on your smpp-tlv groups, etc.

So far so good, in fact you could pass this kind of stuff on regular url variables, but the goal of this patch was to allow to add stuff back from the smsc's response.

Now, with patch #2 also applied, the url returned would be something like this:

http://localhost/x?md=%3Fsmpp%3Fmy_own_field%3D1234%26dlr_status%3D69%26

The "md" parameter, once urldecoded would look like:

?smpp?my_own_field=1234&dlr_status=69&

In this case, dlr_status gets loaded with submit_sm_resp's command_status which was: 69 = 0x00000045 (Submit Failed).

The "sequence_number" could be added just as easily (not a bad idea imho). What do you think? "dlr_sequence" or "dlr_seq" would be ok?

I'm writing the userguide patch if this goes forward of course.

Comments?

Regards,
--
Alejandro Guerrieri
[email protected]














--- End Message ---

Regards,
--
Alejandro Guerrieri



On 06/12/2009, at 12:41, Alexander Malysh wrote:

Hi Alex,

sorry for delay...

Patch looks OK but I don't like this part:
+            if (msg->sms.meta_data == NULL)
+                msg->sms.meta_data = octstr_create("");
+            meta_data_set_value(msg->sms.meta_data, "smpp", octstr_imm("dlr_status"),
+                                octstr_format("0x%08lx", pdu->u.submit_sm_resp.command_status), 1);

Why do you need to forward SMPP internal status to client? If all was fine and message was accepted it's always 0 and if
it was rejected you will receive errorcode in DLR msgdata field.
Therefore I don't really know why you need this part?

I'm +1 for this patch without dlr_status part.

Thanks,
Alexander Malysh

P.S. Don't forget to rebase your patch due to recent changes in dlr_pgsql.c :)

Am 05.11.2009 um 23:41 schrieb Alejandro Guerrieri:

This is an expanded version of a patch I've done a couple of months ago. It adds the following functionality and fix a few things in the process:

#1: Allows meta-data to be passed when sending a message to come back on the DLR's (internal, intermediate and final).

***COMPATIBILITY BREAKER***
A new "meta-data" field is needed on the DB table.
***COMPATIBILITY BREAKER***

This allows, for example, to set the dlr-url on the smsbox group and then pass extra parameters as meta-data (either on the ?smpp? group or you could even create your own, ?dlr? for example).

#2: It creates a meta data value called "dlr_status" that comes back on the internal dlr generated by kannel. The loaded value is the SMPP "command_status" parameter.

#3: It fixes/cleanup code on some dlr_<dbengine>.c, for example on many places %s was used inside octstr_format, and then octstr_get_cstr(var) was used, where using %S would be more direct:

-        sql = octstr_format("DELETE FROM %s WHERE %s='%s' AND %s='%s' %s",
-                            octstr_get_cstr(fields->table),
-                            octstr_get_cstr(fields->field_smsc), octstr_get_cstr(smsc),
-                            octstr_get_cstr(fields->field_ts), octstr_get_cstr(ts), sdb_get_limit_str());

+        sql = octstr_format("DELETE FROM %S WHERE %S='%S' AND %S='%S' %s",
+                            fields->table, fields->field_smsc, smsc,
+                            fields->field_ts, ts, sdb_get_limit_str());
 
#4 There was also a "LIMIT 1" on dlr_sdb.c where sdb_get_limit_str() should be used instead.

The "md" parameter, once urldecoded would look like:

?smpp?my_own_field=1234&dlr_status=69&

You can pass as many parameters as you want of course and they would be added to meta-data along with any other fields you've defined on your smpp-tlv groups, etc. You're not limited to ?smpp?, you can add your own meta-data groups to avoid possible conflicts.

Regarding the dlr_status, in this example gets loaded with submit_sm_resp's command_status which was: 69 = 0x00000045 (Submit Failed).

Please review, I'm writing the userguide part if it goes forward.

Regards,
--
Alejandro Guerrieri

<kannel-dlr-meta-data.diff.zip>



Reply via email to