Hi!
On Feb 22, antony@stripped wrote:
Please see below.
Mostly questions, a couple of suggestions about comments, and a
discussion about the proper place for mysql_audit_release().
> diff -Nrup a/plugin/audit_null/plug.in b/plugin/audit_null/plug.in
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/plugin/audit_null/plug.in 2008-02-22 08:56:22 -08:00
> @@ -0,0 +1,4 @@
> +MYSQL_PLUGIN(audit_null, [NULL Audit Plug-in],
> + [Simple black-hole Audit example plug-in])
> +MYSQL_PLUGIN_DYNAMIC(audit_null, [adt_null.la])
> +MYSQL_PLUGIN_STATIC(audit_null, [libadtnull.a])
I hope, you've tried both static and dynamic builds ?
> diff -Nrup a/plugin/audit_null/audit_null.c b/plugin/audit_null/audit_null.c
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/plugin/audit_null/audit_null.c 2008-02-22 08:56:21 -08:00
> @@ -0,0 +1,136 @@
.....
> +#include <stdio.h>
> +#include <mysql/plugin.h>
> +#include <mysql/plugin_audit.h>
why did you decide not to include plugin_audit.h from plugin.h ?
> +
> +#if !defined(__attribute__) && (defined(__cplusplus) || !defined(__GNUC__)
> || __GNUC__ == 2 && __GNUC_MINOR__ < 8)
> +#define __attribute__(A)
> +#endif
> +
> +static volatile int number_of_calls; /* for SHOW STATUS, see below */
> +
> diff -Nrup a/include/mysql/plugin_audit.h b/include/mysql/plugin_audit.h
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/include/mysql/plugin_audit.h 2008-02-22 08:56:22 -08:00
> @@ -0,0 +1,78 @@
.....
> +#define MYSQL_AUDIT_CLASS_MASK_SIZE 1
> +typedef unsigned long long mysql_audit_mask_t[MYSQL_AUDIT_CLASS_MASK_SIZE];
> +
> +struct st_mysql_audit
> +{
> + int interface_version;
> + const mysql_audit_mask_t *class_mask;
sorry, I don't understand - a pointer to an array ?
You had a pointer to ulonglong, I suggested to change a pointer to
an array. But now you have a pointer to an array. One dereference too much.
> + void (*release_thd)(MYSQL_THD);
> + void (*event_notify)(MYSQL_THD, const struct mysql_event *);
> +};
> +
> +
> +#endif
> diff -Nrup a/sql/Makefile.am b/sql/Makefile.am
> --- a/sql/Makefile.am 2007-12-21 11:27:42 -08:00
> +++ b/sql/Makefile.am 2008-02-22 08:56:19 -08:00
> @@ -130,7 +130,7 @@ mysqld_SOURCES = sql_lex.cc sql_handler.
> event_queue.cc event_db_repository.cc events.cc \
> sql_plugin.cc sql_binlog.cc \
> sql_builtin.cc sql_tablespace.cc partition_info.cc \
> - sql_servers.cc sha2.cc
> + sql_servers.cc sql_audit.cc sha2.cc
oops, bad merge. You lost sha2.cc
>
> if HAVE_DTRACE
> mysqld_SOURCES += probes.d
> diff -Nrup a/sql/mysqld.cc b/sql/mysqld.cc
> --- a/sql/mysqld.cc 2008-02-14 01:00:48 -08:00
> +++ b/sql/mysqld.cc 2008-02-22 08:56:20 -08:00
> @@ -1239,6 +1240,7 @@ extern "C" void unireg_abort(int exit_co
> clean_up(!opt_help && (exit_code || !opt_bootstrap)); /* purecov:
> inspected */
> DBUG_PRINT("quit",("done with cleanup in unireg_abort"));
> wait_for_signal_thread_to_end();
> + mysql_audit_finalize();
why did you move it from clean_up_mutexes() ?
> clean_up_mutexes();
> my_end(opt_endinfo ? MY_CHECK_ERROR | MY_GIVE_INFO : 0);
> exit(exit_code); /* purecov: inspected */
> @@ -4313,6 +4325,7 @@ int main(int argc, char **argv)
> #endif
> clean_up(1);
> wait_for_signal_thread_to_end();
> + mysql_audit_finalize();
Cannot you have mysql_audit_finalize() in one place ?
> clean_up_mutexes();
> my_end(opt_endinfo ? MY_CHECK_ERROR | MY_GIVE_INFO : 0);
>
> diff -Nrup a/sql/scheduler.cc b/sql/scheduler.cc
> --- a/sql/scheduler.cc 2007-12-13 09:17:25 -08:00
> +++ b/sql/scheduler.cc 2008-02-22 08:56:20 -08:00
> @@ -666,6 +667,8 @@ static bool libevent_needs_immediate_pro
> static void libevent_thd_add(THD* thd)
> {
> char c=0;
> + /* release any audit resources, this thd is going to sleep */
> + mysql_audit_release(thd);
Why do you need it here ?
1. Why not to do it in do_command(), at the end of the statement processing,
that is ?
2. Having it in libevent_thd_add implies that it depends on the scheduler.
We'll need to patch every new scheduler that could be created, then ?
> pthread_mutex_lock(&LOCK_thd_add);
> /* queue for libevent */
> thds_need_adding= list_add(thds_need_adding, &thd->scheduler.list);
> diff -Nrup a/sql/sql_audit.h b/sql/sql_audit.h
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/sql/sql_audit.h 2008-02-22 08:56:21 -08:00
> @@ -0,0 +1,51 @@
> +/* Copyright (C) 2007 MySQL AB
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; version 2 of the License.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
> +
> +
> +#include <mysql/plugin_audit.h>
> +
> +extern mysql_audit_mask_t mysql_global_audit_mask;
comments, please
> +
> +
> +extern void mysql_audit_initialize();
> +extern void mysql_audit_finalize();
> +
> +
> +extern void mysql_audit_init_thd(THD *thd);
> +extern void mysql_audit_free_thd(THD *thd);
> +
> +
> +extern void mysql_audit_notify(THD *thd, uint event_class,
> + uint event_subtype, ...);
> +extern void mysql_audit_release(THD *thd);
> +
> +
> +
> +static inline
please, explain all the arguments in a comment
> +void mysql_audit_general(THD *thd, uint event_subtype,
> + int error_code, time_t time,
> + const char *user, uint userlen,
> + const char *cmd, uint cmdlen,
> + const char *query, uint querylen,
> + CHARSET_INFO *clientcs,
> + ha_rows rows)
> +{
> +#ifndef EMBEDDED_LIBRARY
> + if (mysql_global_audit_mask[0] & MYSQL_AUDIT_GENERAL_CLASSMASK)
> + mysql_audit_notify(thd, MYSQL_AUDIT_GENERAL_CLASS, event_subtype,
> + error_code, time, user, userlen, cmd, cmdlen,
> + query, querylen, clientcs, rows);
> +#endif
> +}
> diff -Nrup a/sql/sql_class.cc b/sql/sql_class.cc
> --- a/sql/sql_class.cc 2008-02-12 09:29:06 -08:00
> +++ b/sql/sql_class.cc 2008-02-22 08:56:20 -08:00
> @@ -873,6 +875,7 @@ THD::~THD()
> cleanup();
>
> ha_close_connection(this);
> + mysql_audit_release(this);
why ? you have mysql_audit_free_thd() below anyway.
> plugin_thdvar_cleanup(this);
>
> DBUG_PRINT("info", ("freeing security context"));
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h 2008-02-01 06:08:44 -08:00
> +++ b/sql/sql_class.h 2008-02-22 08:56:20 -08:00
> @@ -1738,6 +1739,11 @@ public:
>
> #ifdef WITH_PARTITION_STORAGE_ENGINE
> partition_info *work_part_info;
> +#endif
> +
> +#ifndef EMBEDDED_LIBRARY
> + DYNAMIC_ARRAY audit_class_plugins;
> + mysql_audit_mask_t audit_class_mask;
comments please
> #endif
>
> THD();
> diff -Nrup a/sql/sql_audit.cc b/sql/sql_audit.cc
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/sql/sql_audit.cc 2008-02-22 08:56:21 -08:00
> @@ -0,0 +1,448 @@
.....
> + /*
> + Check if this plugin may already be registered. This will fail to
> + acquire a newly installed plugin on a specific corner case where
> + one or more event classes already in use by the calling thread
> + are an event class of which the audit plugin has interest.
I have troubles understaind it :(
(need to re-read my review comment to remember what it is about)
please try to rephrase it
> + */
> + if (!check_audit_mask(*data->class_mask, thd->audit_class_mask))
> + return 0;
> +
Regards / Mit vielen Grüssen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Developer/Server Architect
/_/ /_/\_, /___/\___\_\___/ MySQL GmbH, Dachauer Str. 37, D-80335 München
<___/ Geschäftsführer: Kaj Arnö - HRB
München 162140
