On 27 Feb, 2008, at 12:52, Sergei Golubchik wrote:
> 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 ?
yes. I even ran entire test suite with static and also ran entire test
suite loading audit_null dynamically.
>
>> 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 ?
I hope that *eventually* there may be many plugin types and I wouldn't
want to force rebuild of unrelated plugin simply because a different
plugin interface has changed.
>> +
>> +#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.
Pointer to an array, same amount of dereferencing as original pointer
to ulonglong except that array is now not unbounded.
>> + 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
No, I see it there.
>
>>
>> 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 ?
It seemed to interfere with my aesthetic sense having it in
clean_up_mutexes() because all the other lines in that function are
pthread_mutex_destroy() calls.
>
>> 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 ?
minor optimization, don't bother releasing audit plugins when you're
busy executing statements. releasing is deferred until thd is idle.
reduces overall overhead, especially in a busy machine.
>
>> 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
ack
>
>> +
>> +
>> +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
ack
>
>> +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.
ok I can remove this.
>
>> 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
ack
>
>> #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
ok - I will try to phrase it better,
>
>> + */
>> + 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
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>
