[Home]

Summary:ASTERISK-07115: [patch][post 1.4] cdr_csv log files splitting
Reporter:Sergey Basmanov (sb)Labels:
Date Opened:2006-06-07 02:52:40Date Closed:2007-07-06 14:14:52
Priority:MajorRegression?No
Status:Closed/CompleteComponents:CDR/cdr_csv
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) cdr_csv2.diff
Description:Added two options in cdr.conf:
[csv]
;hourly - filename format: yyyy-mm-dd-hour-hh.csv
;daily - filename format: yyyy-mm-dd.csv
;weekly - filename format: yyyy-mm-week-ww.csv
;monthly - filename format: yyyy-mm.csv
;if not set, Master.csv is used by default
splitmode=daily

;write separate cdr_csv log files for each accountcode (default - yes)
separatelogs=no
Comments:By: Andrey S Pankov (casper) 2006-06-07 19:48:40

I'd better have something like this:

[csv]
;logmode = split     ; Default is 'master,split'
;splitby = userfield ; Default is 'accountcode'

;masterfile = %Y-%m-%d     ; Use Master-YYYY-mm-dd.csv as master file name format
                          ; (default is Master.csv)
;splitfile = %Y-%m-bighost ; Use ${CDR('splitby')}-YYYY-mm-bighost.csv as splitted
                          ; file name format (default is ${CDR('splitby')}.csv)
By: Sergey Basmanov (sb) 2006-06-11 16:22:37

Hmmm... Looks great.
I'll upload patch soon. Thanks for looking.
By: Serge Vecher (serge-v) 2006-06-16 16:07:38

ping*ping
By: Sergey Basmanov (sb) 2006-06-17 19:15:20

Sorry, had no free time to finish this.
So, code as casper suggested. Unfortunately, I haven't tested it yet.
By: Sergey Basmanov (sb) 2006-06-18 08:57:11

Small tweaks. Please, delete cdr_csv2.diff
By: Andrey S Pankov (casper) 2006-06-18 10:43:13

sb: formatting!!!!

>mode_master=1;
>if(mode_master) {
...
By: Sergey Basmanov (sb) 2006-06-18 11:16:24

Sorry about that. I always missing something..
Seems now it should be ok.
Thanks for looking.
By: Andrey S Pankov (casper) 2006-06-18 13:04:41

Still probems with formatting ;)

while((tmp2 = strsep(&tmp1, ","))) {
if(!strcasecmp(tmp2, "master")) {
mode_master ? "master":"", mode_split ? "split":""
ast_log(LOG_DEBUG, "master log file '%s'\n",splitby);
ast_log(LOG_DEBUG, "log files split by '%s'\n",splitby);
if(ret) {
By: Sergey Basmanov (sb) 2006-06-19 08:24:09

Hope it ok now.
Please, someone, delete all other files? cdr_csv5.diff is latest.
By: Serge Vecher (serge-v) 2006-06-19 11:17:02

sb: see if you can find some testers for this feature to try it and report results here.
By: Andrey S Pankov (casper) 2006-06-19 12:24:41

+static char mode_master = 1;
+static char mode_split = 1;
Why not static unsigned int?

splitby = ast_strdupa(tmp);
masterfile = ast_strdupa(tmp);
splitfile = ast_strdupa(tmp);
Sure? Not ast_strdup? I'm a bit unsure here...

+ if (mode_master) {
+ if (mode_split) {
Any chance not to duplicate code here?

Won't it leak memory on splitby, masterfile and splitfile on reload etc?...
By: Sergey Basmanov (sb) 2006-06-19 15:38:02

>>Why not static unsigned int?
Oh, i thought it was uint. Fixed.

>>Sure? Not ast_strdup? I'm a bit unsure here...
Me too. So, I changed it.

>>+ if (mode_master) {
>>+ if (mode_split) {
>>Any chance not to duplicate code here?
It seems to me this is fastest way to write to two files. Original version of cdr_csv calls function to write 'accountcode' file. I just joined code in one place.

>>Won't it leak memory on splitby, masterfile and splitfile on reload etc?...
I thought about this. Yes, it will leak memory on reload.
Take a look at new diff I attached.
By: Sergey Basmanov (sb) 2006-06-19 15:40:20

vechers: I'll try to test it myself as soon as I will have my secondary server (with trunk version) up and running. Then I post results here.
By: Andrey S Pankov (casper) 2006-06-19 15:59:08

Please change strncpy() to ast_copy_string() and memset()s are not needed then...

+ tmp1 = ast_strdup(tmp);
...
+ if (tmp1) {
+ free(tmp1);
??? Not ast_strdupa()? Without free()?

+ if (!mf) {
+ ast_log(LOG_ERROR, "Unable to re-open master file %s : %s\n", csvmaster, strerror(errno));
+ }
+ if (mf) {
Why not to use 'else'???

And for your future coding practice - strncpy needs 'sizeof(buf)-1'...

Tired... :(
By: Sergey Basmanov (sb) 2006-06-19 18:27:44

>>+ tmp1 = ast_strdup(tmp);
>>...
>>??? Not ast_strdupa()? Without free()?

Well, I'm not sure which one should I use. ast_strdup is a wrapper for strdup, while ast_strdupa calls __builtin_alloca. strdup using malloc to allocate memory and _builtin_alloca allocates memory in stack (which is freed automatically).
Ok, I changed back to ast_strdupa() (at least it doesn't require to free() memory).
One more question: should I check if ${CDR('splitby')} contains '/' or '.' as it was done in original function?
if (strchr(acc, '/') || (acc[0] == '.')) {
ast_log(LOG_WARNING, "Account code '%s' insecure for writing file\n", acc);
return -1;
}


By: Sergey Basmanov (sb) 2006-06-20 10:20:54

vechers: Please, delete cdr_csv.diff and cdr_csv5.diff
Thank You.
By: Sergey Basmanov (sb) 2006-06-24 16:32:27

Fixed some bugs. This patch is works for me.
Please, delete cdr_csv6.diff
By: Andrey S Pankov (casper) 2006-06-25 17:26:27

+ strcpy(masterfile, "");
+ strcpy(splitfile, "");
and so on... this is too bad... :)
By: Serge Vecher (serge-v) 2006-06-28 15:20:57

marking this as [post 1.4] to considere for trunk once 1.4 branch is forked very soon.
By: Serge Vecher (serge-v) 2006-06-28 15:25:20

sb: also, as per CODING GUIDELINES, single line if or if ... else/if ... constructs don't need curly brackets as in"
+ if (!strcasecmp(tmp2, "master")) {
+ mode_master = 1;
+ } else if (!strcasecmp(tmp2, "split")) {
+ mode_split = 1;
+ } else {
+ ast_log(LOG_DEBUG, "unknown logmode '%s'\n", tmp2);
+ }

->

+ if (!strcasecmp(tmp2, "master"))
+ mode_master = 1;
+ else if (!strcasecmp(tmp2, "split"))
+ mode_split = 1;
+ else
+ ast_log(LOG_DEBUG, "unknown logmode '%s'\n", tmp2);

and so on throughout the patch.
By: jmls (jmls) 2006-10-31 12:39:26.000-0600

can we have an updated patch against trunk and with the coding changes suggested by serge-v ? Thanks
By: Sergey Basmanov (sb) 2006-10-31 14:12:33.000-0600

Oh, I almost forgot about this...
Ok, will provide updated patch in a day or two.
By: Sergey Basmanov (sb) 2006-11-25 06:26:48.000-0600

Sorry for long delay.
So, reworked as requested. Let me know if there is something wrong.
Thank You.
By: Sergey Basmanov (sb) 2006-11-25 06:34:28.000-0600

This one more correct. Please, delete all other files.
By: Steve Murphy (murf) 2007-07-06 14:14:44

sb-- I really don't want to get you upset or anything, but I have some reservations about this enhancement. First of all, there are several backends, and this patch only applies to one. On the other hand, there is only one other backend to a text file, and that would be cdr_custom. I'm sure it would be trivial to apply this patch to cdr_custom also... My next reservation is that this could fairly easily be done by a fairly simple script or two, to generate a set of files for any field, sorting all CDR's into one of the files based on a field value. I'd probably be tempted to hit it with gawk or perl or something.

Anyway, I'm just not convinced that there's enough support for this... I'm going to close it for now, but if you can drum up maybe 4-5 other developers that want it, too, let's re-open and commit this, I'll even apply it cdr_custom for you, in that case. OK?