Summary:	ASTERISK-06222: [patch] Convert cdr_addon_mysql to use custom cdr format
Reporter:	Edward Eastman (whisk)	Labels:
Date Opened:	2006-01-31 03:41:36.000-0600	Date Closed:	2011-06-07 14:03:23
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	CDR/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) cdr_addon_mysql_ver5.c ( 1) cdr_mysql2.conf
Description:	This is an updated version of cdr_addon_mysql using the custom cdr variables as in cdr_custom. It uses cdr_mysql_custom.conf for it's settings. ****** ADDITIONAL INFORMATION ****** This is my first submission and I'm no c guru - it's a hacked together version of cdr_custom and cdr_addon_mysql, and while I understand what's going on overall, the finer points of memory allocation/asterisk internals etc are still rather lost on me.  Having said that it does work for me :) To get this to compile I had to copy asterisk.h from my asterisk source/include directory into the addons folder - It wasn't in my /usr/include/asterisk folder - so i'm not quite sure on the correct way to find it.
Comments:	By: Edward Eastman (whisk) 2006-01-31 03:55:27.000-0600 cdr_addon_mysql_ver2.c uploaded to do module unload properly (replaces cdr_addon_mysql.c) By: Edward Eastman (whisk) 2006-01-31 16:03:27.000-0600 cdr_addon_mysql_ver3.c allows multiple sql statements to be executed for each cdr.   They should be semi-colon separated, which must itself be escaped - e.g. master.sql => INSERT INTO cdr (calldate, clid, ... uniqueid) VALUES ('${CDR(start)}', '${CDR(clid)}', ... '${CDR(uniqueid)}')\; INSERT INTO test (test2, test3) VALUES ('${CDR(uniqueid)}', '${CDR(clid)}')\; UPDATE cdr SET accountcode = '456' WHERE uniqueid='${CDR(uniqueid)}' The mysql docs don't seem to say whether or not allowing multiple statement execution has any performance penalty for single statements, but if so it should probably be a config file option, which wouldn't be too hard to implement. By: Kaloyan Kovachev (knk) 2006-02-01 02:18:35.000-0600 I think you are mixing the two possible options of having separate (new) add-on for the custom CDR and changing the current one to do this. For the CLI you are using the same command, which will interfere with the current module, but your code doesn't implement the old behavior. It is probably beter to keep the same module and behavior if [mappings] (or [custom]) section is not defined and to use that custom SQL statement otherwise (for backward compatibility). By: Joseph Benden (jbenden) 2006-02-02 11:25:47.000-0600 Hello, I just have two comments. First, I think there should be a mutex around the entire load_config() routine. It is possible for two threads to reload and for a currently running CDR to totally bomb by the CDR thread reading half written configuration strings and two reload threads stomping the configuration strings... I'm thinking it could become crazy... Second, I'm also thinking that if they reload the configuration, the database connection should be forcefully closed and reopened.  The user might have changed the DB connection parameters, but we won't get those unless the current DB connection dies, etc... Thoughts, comments, suggestions? Thanks. By: Edward Eastman (whisk) 2006-02-09 17:19:37.000-0600 Thanks for your comments - KNK you're right I'm not sure whether to add a new module or replace the existing one and i've probably done half of both (or the worst of both worlds ;) I'm looking for guidance over which is the best option really. JBenden I think you're definately right about bouncing the DB connection on a reload - although I'm unsure about the mutex (as in I don't know, not I'm not sure if you're right or not) - either way what's there at the moment is silly because there's a mutex around some of it but not all - I'll try and upload a new version tomorrow. I'm happy to fax a disclaimer if it's necessary, but I'd prefer to explicitly limit it to code posted by me under this mantis username, because I don't want to get into a mess with (asterisk) code done for my employer which might not be disclaimed (although obviously I'll try and persuade them of the benifits of returning code to the community etc) and code done by me outside work which i do want to.  Can someone confirm whether the format suggested in http://bugs.digium.com/view.php?id=6442 is acceptable. By: Edward Eastman (whisk) 2006-02-13 16:25:49.000-0600 cdr_addon_mysql_ver4.c has implemented JBenden's suggestions - there is a lock around the load_config and the database connection is now dropped and reconnected on a reload. By: Kaloyan Kovachev (knk) 2006-02-28 09:23:40.000-0600 I see its going to be a new module, so change "cdr mysql status" to "cdr mysql_custom status" (lines 84-88 and 124-127) and all log messages from "cdr_mysql" to "cdr_mysql_custom". In load_config (line 141) you are locking inside the while lood instead of before the loop. Also skip the locking (in load_config) when reload = 1, as it is allready locked from reload itself. By: Edward Eastman (whisk) 2006-03-05 15:46:39.000-0600 ver5 supercedes all others (which can be removed) and implements KNK's suggestions By: Tilghman Lesher (tilghman) 2006-05-06 09:30:04 Is this backward compatible to the current cdr_addon_mysql.c, or is this to be added as a new module? By: Joseph Benden (jbenden) 2006-05-06 11:40:26 In my opinion, with the same SQL mapping defined, it works the same way as cdr_addon_mysql, but the configuration of this is slightly different.  Thus, some form of note would be needed for people to be aware that they would have to reconfigure this to make it work during an upgrade. The code is cleaner and easier to maintain.  I vote for this one, over the previous version. I only have one final concern about this.  During the variable substitution, I think the variables should have been "preprocessed" with mysql_real_escape; otherwise it could be possible to generate an invalid query that would fail against the database.  I'm not seeing how the variables would have been properly escaped, such that a caller id name of "Joseph's Phone" would be replaced into the query as 'Joseph's Phone' causing an error.  (This would also allow for a hidden SQL injection attack off of the caller id name.) By: Serge Vecher (serge-v) 2006-05-06 13:06:58 Whisk: can you please fax your disclaimer to Digium? I've looked at your disclaimer note in bug 6442 and looks like you've used a short version of the disclaimer specifically for your personal contributions. I believe this should be fine. Thanks. By: Edward Eastman (whisk) 2006-05-06 17:18:55 I'll fax over a disclaimer on Monday.  The current file isn't quite backwards-compatible with the old cdr_addon_mysql but it wouldn't be too hard to make it so (although it would add a bit of bloat)- I can have a go if that seems like a good idea. JBenden has quite right about the sql injection possibility, there's a couple of ways we could fix this, but I think the best way would be to add a dialplan function in cdr_mysql to escape a string (have i seen this somewhere else already? - it rings a bell but that may just have been me thinking about this before) that way you can have an insert statement like: INSERT INTO cdr(calldate, clid,...) VALUES ('${CDR(start)}', '${MYSQL_ESCAPE(${CDR(clid)})}'...) By: Tilghman Lesher (tilghman) 2006-05-07 00:16:55 The dialplan function is called SQL_ESC and is only available if func_odbc.so is loaded. Also, given how different this module is, I think I would prefer to make it simply a different module altogether, so those using the current cdr_addon_mysql.so are not surprised.  If it was backward compatible and fell back to working the way the current module works, I could see it replacing the current module.  However, given that it is maintained outside of the main tree, I think that it's best that it become a whole new module. By: () 2006-05-09 11:44:52 Between 3 and 5 May I packaged this into an addons.rpm and tested it. I might be missing something obvious, but I couldn't get it to work. Asterisk would try to load both mysql modules (cdr and cdr_custom) and fail completely, i.e. not start at all. This got me wondering about the point of making this a separate module (or the need to keep cdr_mysql when cdr_mysql_custom makes it to the stable tree). Anything that cdr_mysql can do, cdr_mysql_custom can do too. Thus, why not add the standard insert statement of cdr as the default of cdr_custom, make it overriddable by [mappings] if any, and simply replace the old module with the new one under the old name? This would eliminate compatibility issues with the existing install base as well as prevent the two modules from colliding with one another. By: Tilghman Lesher (tilghman) 2006-05-22 10:50:57 zenon:  that would be acceptable, if someone would care to update the patch. By: () 2006-05-28 02:17:04 Sadly, I'm not even half as fluent in C as anyone around here is in Swahili, so I don't touch code. However, thinking more of it, I suspect it would be better to import the new functionality of cdr_custom_mysql into the old cdr_mysql than to import the new module as such into asterisk. Basically, getting the [mappings] functionality into the old module should achieve the desired goal and do away with module collisions at the same time. By: Tilghman Lesher (tilghman) 2006-06-12 15:28:21 There appears to be no interest in updating this patch.  Reopen if/when we have an update for this patch that makes it backwards compatible with the existing module.