MDEV-18386: Add server_audit_timestamp_format to customize audit log timestamps#4633
MDEV-18386: Add server_audit_timestamp_format to customize audit log timestamps#4633abhishek593 wants to merge 8 commits intoMariaDB:mainfrom
Conversation
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
This is a preliminary review. Thank you for your contribution!
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
it was a good first iteration. Thanks for working on that. Another round of comments coming up.
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM after fixing the below. Please expect a final review.
plugin/server_audit/server_audit.cc
Outdated
| NULL, NULL, 1024, 0, 0x7FFFFFFF, 1); | ||
| static MYSQL_SYSVAR_STR(timestamp_format, timestamp_format, | ||
| PLUGIN_VAR_RQCMDARG, | ||
| "The format string the strftime() routine applies with its format argument", |
There was a problem hiding this comment.
I'd say: a format string used to print the timestamp into the audit log messages. The format used is the same as strftime. Default is empty, meaning use the existing hard-coded format.
plugin/server_audit/server_audit.cc
Outdated
| if (timestamp_format_buffer[0]) | ||
| ts_len= strftime(ts_tmp, sizeof(ts_tmp), timestamp_format_buffer, &tm_time); | ||
|
|
||
| if (ts_len == 0) |
There was a problem hiding this comment.
just do "else" here. Easier to read.
plugin/server_audit/server_audit.cc
Outdated
| { | ||
| if (!(is_active= (logger_write(logfile, message, len) == (int) len))) | ||
| struct tm tm_time; | ||
| char ts_tmp[TIMESTAMP_OUTPUT_LENGTH]; |
There was a problem hiding this comment.
256 bytes in the stack ... this is a bit much. I'd allocate a buffer outside of the lock and pass it down. Or, better, it's going at the start of message anyway, can't you just print it there directly instead of using a temp buffer and then memcpy() into it?
There was a problem hiding this comment.
Yeah, so here's what I changed. Removed the temp buffer, and directly stored the output of strftime in message buffer at the beginning, then used memmove to shift the output from the start to the end of reserved space.
plugin/server_audit/server_audit.cc
Outdated
| /** | ||
| Write to the log, acquiring the lock. | ||
| */ | ||
| */ |
There was a problem hiding this comment.
don't do white-space only changes please!
plugin/server_audit/server_audit.cc
Outdated
| const char *userip, unsigned int userip_len, | ||
| unsigned int connection_id, unsigned int port, | ||
| long long query_id, const char *operation) | ||
| const char *serverhost, size_t serverhost_len, |
There was a problem hiding this comment.
no white-space only changes please
plugin/server_audit/server_audit.cc
Outdated
| time_t ctime; | ||
| size_t csize; | ||
| char message[1024]; | ||
| char raw_message[1024 + TIMESTAMP_OUTPUT_LENGTH]; |
There was a problem hiding this comment.
I'd use some named const size_t for the 1k constant
plugin/server_audit/server_audit.cc
Outdated
|
|
||
| (void) time(&ctime); | ||
| csize= log_header(message, sizeof(message)-1, &ctime, | ||
| csize= log_header(message, 1024-1, |
There was a problem hiding this comment.
why do you need to do this? Can't you just pass the actual length as you used to do? And then do the calculation inside this func?
There was a problem hiding this comment.
Previously message was a fixed size array (1024). In the new version, its a pointer to an offset within a large buffer, so we must explicitly pass the correct size (MAX_AUDIT_PAYLOAD_LENGTH).
plugin/server_audit/server_audit.cc
Outdated
| time_t ctime; | ||
| size_t csize; | ||
| char message[1024]; | ||
| char raw_message[1024 + TIMESTAMP_OUTPUT_LENGTH]; |
There was a problem hiding this comment.
same here: use a named constant for 1k
3 participants
This PR adds a new global system variable, server_audit_timestamp_format, to the MariaDB Audit Plugin.
Key changes: