Detail file reader header detection sucks
Hi, src/modules/frs_detail/frs_detail.c 650 /* 651 * Look for date/time header, and read VP's if 652 * found. If not, keep reading lines until we 653 * find one. 654 */ 655 if (data->state == STATE_HEADER) { 656 int y; 657 658 if (sscanf(buffer, "%*s %*s %*d %*d:%*d:%*d %d", &y)) { 659 data->state = STATE_READING; 660 } 661 continue; 662 } This'd be fine, except the format of the detail file header is user definable! Wouldn't it be safer to just assume that the first line that doesn't start with a \t is the header? if (data->state == STATE_HEADER && (buffer[0] != '\t')) { continue; } It's not like the detail file listener actually does anything with the data from the record header. Best regards, Arran
Arran Cudbard-Bell wrote:
src/modules/frs_detail/frs_detail.c ... This'd be fine, except the format of the detail file header is user definable!
Sure... for use *other* than with the detail file reader.
Wouldn't it be safer to just assume that the first line that doesn't start with a \t is the header?
Maybe. Unless the detail file is corrupted somehow.
if (data->state == STATE_HEADER && (buffer[0] != '\t')) { continue; }
It's not like the detail file listener actually does anything with the data from the record header.
Well, yes. Alan DeKok.
Alan DeKok wrote:
Arran Cudbard-Bell wrote:
src/modules/frs_detail/frs_detail.c
...
This'd be fine, except the format of the detail file header is user definable!
Sure... for use *other* than with the detail file reader.
Yeah, but the configuration option still exists. I got caught out changing the %t to a %T, as I was assuming the detail reader used the header for Packet-Original-Timestamp (when in fact it's reformatting the Unix timestamp included in the packet). I need database formatted or unix timestamps for inserting command logs into a database... I can convert c timestamps using unlang, but it's not pretty.
Wouldn't it be safer to just assume that the first line that doesn't start with a \t is the header?
Maybe. Unless the detail file is corrupted somehow.
I guess, but what do you want to do if that happens? Because at the moment if the headers not detected correctly, it just skips on down and uses the first attribute with a timestamp as its value, missing out half the attributes in the process. You then get a request coming in with no 'Acct-Status-Type', and if you haven't got your configuration setup correctly, it'll just loop.
if (data->state == STATE_HEADER && (buffer[0] != '\t')) { continue; }
It's not like the detail file listener actually does anything with the data from the record header.
Well, yes.
I guess this isn't that much of a problem, but I still think just checking for a non-indented line gives more predictable behaviour, even with corruption. Arran
Arran Cudbard-Bell wrote:
Yeah, but the configuration option still exists. I got caught out changing the %t to a %T, as I was assuming the detail reader used the header for Packet-Original-Timestamp (when in fact it's reformatting the Unix timestamp included in the packet).
Yes...
I need database formatted or unix timestamps for inserting command logs into a database... I can convert c timestamps using unlang, but it's not pretty.
Use %S.
I guess, but what do you want to do if that happens? Because at the moment if the headers not detected correctly, it just skips on down and uses the first attribute with a timestamp as its value, missing out half the attributes in the process.
Hmm... that needs to be fixed then.
I guess this isn't that much of a problem, but I still think just checking for a non-indented line gives more predictable behaviour, even with corruption.
OK. Alan DeKok.
Hi,
You then get a request coming in with no 'Acct-Status-Type', and if you haven't got your configuration setup correctly, it'll just loop.
tell me about it..... ;-) after ensuring that the detail module is suitably wrapped in logic cotton I'm quite happy with its day to day behaviour now...the quick \t check seems quite appropriate in this case. alan
participants (3)
-
Alan Buxey -
Alan DeKok -
Arran Cudbard-Bell