Re: [PATCH] Pass meta-data from message to dlrs
Alejandro Guerrieri <[email protected]>
| Newsgroups | gmane.comp.mobile.kannel.devel |
|---|---|
| Message-ID | <[email protected]> |
BTW, while I'm adding the meta_data field on all DB engines, and I've
noticed a difference on dlr_oracle.c, on dlr_get_oracle:
sql = octstr_format("SELECT %S, %S, %S, %S, %S, %S, %S FROM %S
WHERE %S=:1 AND %S=:2 AND %S=:3 AND ROWNUM < 2",
fields->field_mask, fields->field_serv,
fields->field_url, fields->field_src,
fields->field_dst, fields->field_boxc,
fields->table, fields->field_smsc,
fields->field_ts, fields->field_dst);
While on all other engines, field_dst is not used on the condition:
pgsql:
sql = octstr_format("SELECT %s, %s, %s, %s, %s, %s FROM %s WHERE
%s='%s' AND %s='%s' LIMIT 1;",
sdb:
sql = octstr_format("SELECT %s, %s, %s, %s, %s, %s FROM %s WHERE
%s='%s' AND %s='%s' %s", [NO LIMIT CLAUSE]
mysql:
sql = octstr_format("SELECT %S, %S, %S, %S, %S, %S FROM %S WHERE %S=?
AND %S=? LIMIT 1",
and so on.
Any reason why this is done that way? Which one should be fixed?
Regards,
--
Alejandro Guerrieri
[email protected]
On 04/09/2009, at 13:46, Alexander Malysh wrote:
>
> Am 04.09.2009 um 13:05 schrieb Alejandro Guerrieri:
>
>> 2009-09-04 Alexander Malysh <amalysh at kannel.org>
>> * aclocal.m4, configure, configure.in, gw/dlr_mysql.c, gwlib/
>> dbpool_mysql.c,
>> test/test_dbpool.c: implemented select/update function calls
>> for dbpool_mysql. This
>> require mysql prepaid statement support and therefore mysql
>> version >= 4.1.
>> Changed dlr_mysql to use these new function calls. This fixes
>> #258.
>>
>> Would make sense if I update sqlbox to use those calls as well right?
>
> yep
>
>
>>
>> Regards,
>> --
>> Alejandro Guerrieri
>> [email protected]
>>
>>
>>
>> On 04/09/2009, at 9:45, Alexander Malysh wrote:
>>
>>> ahh alex,
>>> before you start to add meta_data to db storages please give me
>>> time to merge mysql-prepaid branch. Just update your cvs in
>>> 30-60min.
>>>
>>> Thanks,
>>> Alexander Malysh
>>>
>>> Am 04.09.2009 um 09:40 schrieb Alexander Malysh:
>>>
>>>> Hi Alex,
>>>>
>>>> if you add:
>>>> + Octstr *meta_data;
>>>> to dlr struct then you have add meta_data column to all storage
>>>> types :) otherwise don't add it. If you ask me, I would say, if
>>>> you add it for faked dlrs then you have
>>>> to add it to db storages as well.
>>>>
>>>> as to the points:
>>>> 1) +0
>>>> 2) -1 because I don't see reason to add internal smpp sequence.
>>>> this is just one counter and will not add any useful information.
>>>> 3) ++1 :)
>>>>
>>>> Thanks,
>>>> Alexander Malysh
>>>>
>>>> Am 04.09.2009 um 01:08 schrieb Alejandro Guerrieri:
>>>>
>>>>> 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.
>>>>>
>>>>> <kannel-dlr-meta-data.diff>
>>>>> <kannel-dlr-command-status.diff>
>>>>>
>>>>> 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
>>>>> [email protected]
>>>>>
>>>>>
>>>>>
>>>>> 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
>>>>>>> [email protected]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>>> [email protected]
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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=%3Fsmpp%3Fmy_own_field%3D1234&dlr-url=http%3A%2F%2Flocalhost%2Fx%3Fdata%3D%25D
>>>>>>>>>
>>>>>>>>> Notes:
>>>>>>>>>
>>>>>>>>> meta-data is urlencoded version of: ?smpp?my_own_field=1234
>>>>>>>>> dlr-url is urlencoded version of: http://localhost/x?data=%D
>>>>>>>>>
>>>>>>>>> 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]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>