On 3 Dec, 2007, at 10:30, Sergei Golubchik wrote:
> Hi!
>
> On Nov 29, antony@stripped wrote:
>> ChangeSet@stripped, 2007-11-29 14:40:36-08:00, acurtis@stripped +34
>> -0
>> WL#3771 - Plugable Audit Interface
>> First commit for review
>> Have run default tests with/without plugin:
>> mysql-test-run.pl --mysqld=--plugin-dir=<path to plugin>
>> --mysqld=--plugin-load=adt_null.so
>
>> include/Makefile.am@stripped, 2007-11-29 14:40:15-08:00, acurtis@stripped
>> +2 -1
>> add new source files: mysql/ftparser.h mysql/audit.h
>
> in wl#4017 patch (query rewrite plugin) BrianM also had a new file for
> the rewrite plugin. But he called it rewrite_plugin.h.
> I think that having _plugin as a part of include file name is a good
> idea - we may have other non-plugin include files someday. Even better
> to have a plugin_ prefix, not suffix, to keep all these files
> together.
> See also my comment to your change in myisam/ha_myisam.cc (below).
>
> Please rename your include files to plugin_ftparser.h and
> plugin_audit.h
Ok,
>
>> include/mysql/ftparser.h@stripped, 2007-11-29 14:40:19-08:00, acurtis@stripped
>> +211 -0
>> New BitKeeper file ``include/mysql/ftparser.h''
>
> It's good that you moved ftparser related code into a separate file.
> Thanks.
>
>> plugin/audit_null/configure.in@stripped, 2007-11-29 14:40:19-08:00,
> acurtis@stripped
>> +9 -0
>> New BitKeeper file ``plugin/audit_null/configure.in''
>
> did you try to build audit_plugin outside of MySQL source tree ?
> if not, and it's not needed (which, I suspect is the case) you don't
> need configure.in, NEWS/AUTHORS/ChangeLog and other files that
> automake
>
>> diff -Nrup a/storage/myisam/ha_myisam.cc b/storage/myisam/
>> ha_myisam.cc
>> --- a/storage/myisam/ha_myisam.cc 2007-10-25 00:39:55 -07:00
>> +++ b/storage/myisam/ha_myisam.cc 2007-11-29 14:40:18 -08:00
>> @@ -20,7 +20,7 @@
>>
>> #define MYSQL_SERVER 1
>> #include "mysql_priv.h"
>> -#include <mysql/plugin.h>
>> +#include <mysql/ftparser.h>
>
> basically, what you've done is a change in the API (but not in ABI) -
> old plugins win't compile anymore, they'll need to be changed.
> I don't think it was necessary. And (I'm looking in a randomly picked
> directory in /usr/include) in libgnome one is expected to write only
> #include <libgnome/libgnome.h> and other 13 include files are
> included by libgnome.h
>
> Let's do the same - it's good to have plugin_ftparser.h,
> plugin_audit.h,
> plugin_rewrite.h, it helps to keep include files structured and
> maintainable.
> But it doesn't need to affect the API. Make all these plugin_*.h
> files to be included from the main plugin.h
Ok,
Original reason for why I did it - I was progressively adding fields
to the audit interface and I did not want to cause recompilation of
all of mysqld again just for adding a couple of members to a struct.
>
>> #include <m_ctype.h>
>> #include <my_bit.h>
>> #include <myisampack.h>
>> diff -Nrup a/include/mysql/plugin.h b/include/mysql/plugin.h
>> --- a/include/mysql/plugin.h 2007-08-30 23:19:49 -07:00
>> +++ b/include/mysql/plugin.h 2007-11-29 14:40:16 -08:00
>> @@ -135,6 +136,7 @@ typedef int (*mysql_show_var_func)(MYSQL
>> #define PLUGIN_VAR_STR 0x0005
>> #define PLUGIN_VAR_ENUM 0x0006
>> #define PLUGIN_VAR_SET 0x0007
>> +#define PLUGIN_VAR_OPAQUE 0x0008
>
> please remove it, as we've discussed on irc
Ok,
>
>> #define PLUGIN_VAR_UNSIGNED 0x0080
>> #define PLUGIN_VAR_THDLOCAL 0x0100 /* Variable is per-
>> connection */
>> #define PLUGIN_VAR_READONLY 0x0200 /* Server variable is read
>> only */
>> @@ -143,6 +145,7 @@ typedef int (*mysql_show_var_func)(MYSQL
>> #define PLUGIN_VAR_NOCMDARG 0x1000 /* No argument for cmd line */
>> #define PLUGIN_VAR_RQCMDARG 0x0000 /* Argument required for cmd
>> line */
>> #define PLUGIN_VAR_OPCMDARG 0x2000 /* Argument optional for cmd
>> line */
>> +#define PLUGIN_VAR_NOGLOBAL 0x4000 /* per-connection has no
>> global */
>
> please remove it, as we've discussed on irc
Ok
>
>> #define PLUGIN_VAR_MEMALLOC 0x8000 /* String needs memory
>> allocated */
>>
>> struct st_mysql_sys_var;
>> diff -Nrup a/sql/ha_ndbcluster_binlog.cc b/sql/
>> ha_ndbcluster_binlog.cc
>> --- a/sql/ha_ndbcluster_binlog.cc 2007-11-13 03:52:17 -08:00
>> +++ b/sql/ha_ndbcluster_binlog.cc 2007-11-29 14:40:16 -08:00
>> @@ -1610,6 +1615,9 @@ end:
>> if (ndb_extra_logging)
>> ndb_report_waiting(type_str, max_timeout,
>> "distributing", ndb_schema_object->key);
>> +
>> + /* Release any held audit resources */
>> + mysql_audit_release(thd);
>
> why here ?
I added these to occur anywhere where I thought it may block/wait for
a period of time so that it would not cause a situation where a plugin
cannot unload.
<snip>
>
>> }
>> }
>> (void) pthread_mutex_lock(&LOCK_open);
>> diff -Nrup a/sql/scheduler.cc b/sql/scheduler.cc
>> --- a/sql/scheduler.cc 2007-11-02 12:39:00 -07:00
>> +++ b/sql/scheduler.cc 2007-11-29 14:40:17 -08:00
>> @@ -396,6 +397,8 @@ static void libevent_connection_close(TH
>>
>> thd->killed= THD::KILL_CONNECTION; // Avoid error messages
>>
>> + mysql_audit_release(thd);
>> +
>
> why here ? it should be done later, e.g. in thd->cleanup()
> as late as possible, to ensure there can be no more audit events
> after mysql_audit_release()
ok.
<snip>
>
>> pthread_mutex_lock(&di->mutex);
>> }
>> if (di->tables_in_use)
>> diff -Nrup a/sql/sql_builtin.cc.in b/sql/sql_builtin.cc.in
>> --- a/sql/sql_builtin.cc.in 2006-12-30 17:28:54 -08:00
>> +++ b/sql/sql_builtin.cc.in 2007-11-29 14:40:17 -08:00
>> @@ -13,6 +13,7 @@
>> along with this program; if not, write to the Free Software
>> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
>> 02110-1301 USA */
>>
>> +#include <my_global.h>
>
> why ?
The audit plugin event struct has a time_t value. Actually, as the
tree currently stands, it is unnecessary but if I have plugin.h
include the audit interface, then it is neccessary.
>
>> #include <mysql/plugin.h>
>>
>> typedef struct st_mysql_plugin builtin_plugin[];
>> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
>> --- a/sql/sql_class.h 2007-11-22 11:45:22 -08:00
>> +++ b/sql/sql_class.h 2007-11-29 14:40:17 -08:00
>> @@ -1100,6 +1100,10 @@ public:
>> ulong max_client_packet_length;
>>
>> HASH handler_tables_hash;
>> +#ifndef EMBEDDED_LIBRARY
>> + DYNAMIC_ARRAY audit_class_plugins;
>> + uint64 audit_class_mask1;
>
> 1. what does 1 mean in mask1 ?
> 2. both fields need a comment
For the future, when it spills over to requiring 2 or more longlongs
(when we have more than 64 audit classes) then the naming doesn't need
to be modified. 1 signifies the first word in the mask.
>
>> +#endif
>> /*
>> One thread can hold up to one named user-level lock. This
>> variable
>> points to a lock object if the lock is present. See
>> item_func.cc and
>> diff -Nrup a/sql/protocol.cc b/sql/protocol.cc
>> --- a/sql/protocol.cc 2007-11-01 04:32:31 -07:00
>> +++ b/sql/protocol.cc 2007-11-29 14:40:17 -08:00
>> @@ -132,6 +133,12 @@ void net_send_error(THD *thd, uint sql_e
>>
>> DBUG_ASSERT(!thd->spcont);
>>
>> +
>> MYSQL_AUDIT_GENERAL
>> (thd,MYSQL_AUDIT_GENERAL_ERROR,sql_errno,my_time(0),
>> + 0,0,err,err?strlen(err):0,
>> + thd->query,thd->query_length,
>> + thd->variables.character_set_client,
>> + thd->row_count);
>
> this wasn't part of the WL
I suspected that if one wishes to audit queries, then it would be
desirable to be notified if the query had an error or was successful.
>
>> if (net && net->no_send_error)
>> {
>> thd->clear_error();
>> @@ -210,6 +217,12 @@ send_ok(THD *thd, ha_rows affected_rows,
>> NET *net= &thd->net;
>> uchar buff[MYSQL_ERRMSG_SIZE+10],*pos;
>> DBUG_ENTER("send_ok");
>> +
>> + MYSQL_AUDIT_GENERAL(thd,MYSQL_AUDIT_GENERAL_RESULT,0,my_time(0),
>> + 0,0,message,message?strlen(message):0,
>> + thd->query,thd->query_length,
>> + thd->variables.character_set_client,
>> + affected_rows);
>
> this wasn't part of the WL
ditto.
from the worklog "The number of affected/retrieved rows"
>
>> if (net->no_send_ok || !net->vio) // hack for re-parsing queries
>> {
>> diff -Nrup a/sql/sql_connect.cc b/sql/sql_connect.cc
>> --- a/sql/sql_connect.cc 2007-11-13 03:52:18 -08:00
>> +++ b/sql/sql_connect.cc 2007-11-29 14:40:17 -08:00
>> @@ -318,6 +319,9 @@ check_user(THD *thd, enum enum_server_co
>> DBUG_ENTER("check_user");
>> LEX_STRING db_str= { (char *) db, db ? strlen(db) : 0 };
>>
>> + MYSQL_AUDIT_SECURITY(thd,MYSQL_AUDIT_SECURITY_AUTHENTICATE,0,
>> + "USER",&db_str,0,0,0);
>
> it wasn't part of the WL
From the worklog.... "to do security auditing of all security related
operations".
>
>> /*
>> Clear thd->db as it points to something, that will be freed when
>> connection is closed. We don't want to accidentally free a wrong
>> @@ -673,6 +677,8 @@ static int check_connection(THD *thd)
>> #ifdef SIGNAL_WITH_VIO_CLOSE
>> thd->set_active_vio(net->vio);
>> #endif
>> + MYSQL_AUDIT_SECURITY(thd,MYSQL_AUDIT_SECURITY_CONNECT,0,
>> + "CONNECTION",0,0,0,0);
>
> it wasn't part of the WL
From the worklog.... "to do security auditing of all security related
operations".
>
>> if (!thd->main_security_ctx.host) // If TCP/IP connection
>> {
>> @@ -997,6 +1003,10 @@ bool login_connection(THD *thd)
>> void end_connection(THD *thd)
>> {
>> NET *net= &thd->net;
>> +
>> + MYSQL_AUDIT_SECURITY(thd,MYSQL_AUDIT_SECURITY_DISCONNECT,0,
>> + "CONNECTION",0,0,0,0);
>
> it wasn't part of the WL
From the worklog.... "to do security auditing of all security related
operations".
<snip>
>
>> end_connection(thd);
>>
>> end_thread:
>> diff -Nrup a/sql/log.cc b/sql/log.cc
>> --- a/sql/log.cc 2007-11-14 02:01:42 -08:00
>> +++ b/sql/log.cc 2007-11-29 14:40:16 -08:00
>> @@ -4387,6 +4397,33 @@ static void print_buffer_to_file(enum lo
>> VOID(pthread_mutex_lock(&LOCK_error_log));
>>
>> skr= my_time(0);
>> +
>> + {
>> + char user_host_buff[MAX_USER_HOST_SIZE];
>> + THD *thd;
>> + ulong id= 0;
>> + uint user_host_len= 0;
>> +
>> + if ((thd= current_thd))
>> + {
>> + Security_context *sctx= thd->security_ctx;
>> + id= thd->thread_id;
>> + user_host_len= strxnmov(user_host_buff, MAX_USER_HOST_SIZE,
>> + sctx->priv_user ? sctx->priv_user :
>> "", "[",
>> + sctx->user ? sctx->user : "", "] @ ",
>> + sctx->host ? sctx->host : "", " [",
>> + sctx->ip ? sctx->ip : "", "]",
>> NullS) -
>> +
>> user_host_buff;
>> + }
>> + MYSQL_AUDIT_GENERAL(thd, MYSQL_AUDIT_GENERAL_CONSOLE,
>> error_code, skr,
>> + user_host_buff, user_host_len,
>> + (level == ERROR_LEVEL ? "ERROR" :
>> + level == WARNING_LEVEL ? "Warning" :
>> "Note"),
>> + (level == ERROR_LEVEL ? 5 :
>> + level == WARNING_LEVEL ? 7 : 4),
>> + buffer, buffer_length,0,0);
>> + }
>> +
>
> it wasn't part of the WL
Ok, I shall remove this.
>
>> localtime_r(&skr, &tm_tmp);
>> start=&tm_tmp;
>>
>> diff -Nrup a/sql/sql_acl.cc b/sql/sql_acl.cc
>> --- a/sql/sql_acl.cc 2007-11-13 03:52:18 -08:00
>> +++ b/sql/sql_acl.cc 2007-11-29 14:40:17 -08:00
>> @@ -0,0 +1,95 @@
>
> WL (at least what was approved as HLS/LLD) did not need any changes to
> sql_acl.cc. Changes to grants are tracked using general audit class.
> failed privilege checks - using mysql error class.
I see in WL customer requirements "failed login/access information"
and "accessed table/view/other objects name and action"
>
>> diff -Nrup a/include/mysql/audit.h b/include/mysql/audit.h
>> --- /dev/null Wed Dec 31 16:00:00 196900
>> +++ b/include/mysql/audit.h 2007-11-29 14:40:19 -08:00
>> @@ -0,0 +1,126 @@
>> +/* 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 */
>> +
>> +#ifndef _my_audit_h
>> +#define _my_audit_h
>> +
>> +/
>> *************************************************************************
>> + API for Audit plugin. (MYSQL_AUDIT_PLUGIN)
>> +*/
>> +
>> +#include "plugin.h"
>> +
>> +#define MYSQL_AUDIT_INTERFACE_VERSION 0x0100
>> +
>> +
>> +struct mysql_event
>> +{
>> + int event_class;
>> +};
>> +
>> +#ifdef __cplusplus
>> +#define DECLARE_AUDIT_EVENT_DATA(name) \
>> +struct mysql_event_ ## name :public mysql_event {
>> +#else
>> +#define DECLARE_AUDIT_EVENT_DATA(name) \
>> +struct mysql_event_ ## name { \
>> + int event_class;
>> +#endif
>
> There's no need to use different code for C and C++
> please remove the #ifdef
>
>> +#define END_DECLARE_AUDIT_EVENT_DATA }
>
> I would remove DECLARE_AUDIT_EVENT_DATA and
> END_DECLARE_AUDIT_EVENT_DATA
> macros, using struct ... and } in place, as appropriate.
> you don't save or simplify much to justify them.
> (unlike your macro helpers related to MYSQL_xxxVAR_yyy variables,
> they are useful indeed)
This was done to reduce the amount of explicit typecasting done in the
code... I can remove them at the cost of more explicit typecasting.
>
>> +
>> +
>> +/
>> *************************************************************************
>> + AUDIT CLASS : GENERAL
>> +*/
>> +
>> +#define MYSQL_AUDIT_GENERAL_CLASS 0
>> +#define MYSQL_AUDIT_GENERAL_CLASSMASK1 (1 <<
>> MYSQL_AUDIT_GENERAL_CLASS)
>> +#define MYSQL_AUDIT_GENERAL_LOG 0
>> +#define MYSQL_AUDIT_GENERAL_ERROR 1
>> +#define MYSQL_AUDIT_GENERAL_RESULT 2
>> +#define MYSQL_AUDIT_GENERAL_CONSOLE 3
>
> only MYSQL_AUDIT_GENERAL_LOG was requested in this WL
Because result summary was wanted, _ERROR and _RESULT was inferred.
I agree that _CONSOLE was not requested at all.
>
>> +DECLARE_AUDIT_EVENT_DATA(general)
>> + int general_error_code;
>> + unsigned long general_thread_id;
>> + time_t general_time;
>> + const char *general_user;
>> + unsigned int general_user_length;
>> + const char *general_command;
>> + unsigned int general_command_length;
>> + const char *general_query;
>> + unsigned int general_query_length;
>> + struct charset_info_st *general_charset;
>> + unsigned long long general_rows;
>> +END_DECLARE_AUDIT_EVENT_DATA;
>
> why "general_" prefix ?
I was in a verbose mood and since I anticipate extension of structs
for future classes, this makes it clear where the attribute was
declared.
>
>> +
>> +
>> +/
>> *************************************************************************
>> + AUDIT CLASS : SECURITY
>> +*/
>> +
>> +#define MYSQL_AUDIT_SECURITY_CLASS 1
>> +#define MYSQL_AUDIT_SECURITY_CLASSMASK1 (1 <<
>> MYSQL_AUDIT_SECURITY_CLASS)
>
> This wasn't specified in WL. mysql error audit class was, but it's not
> implemented
This was my interpretation of customer requirement for "all security
related operations".
>
>> +#define MYSQL_AUDIT_SECURITY_RELOAD 0
>> +#define MYSQL_AUDIT_SECURITY_AUTHENTICATE 1
>> +#define MYSQL_AUDIT_SECURITY_CONNECT 2
>> +#define MYSQL_AUDIT_SECURITY_DISCONNECT 3
>> +#define MYSQL_AUDIT_SECURITY_CHECK_ENTER 4
>> +#define MYSQL_AUDIT_SECURITY_CHECK_LEAVE 5
>> +#define MYSQL_AUDIT_SECURITY_GRANT 6
>> +#define MYSQL_AUDIT_SECURITY_REVOKE 7
>> +DECLARE_AUDIT_EVENT_DATA(security)
>> + int security_error_code;
>> + unsigned long security_thread_id;
>> + const char *security_host;
>> + unsigned int security_host_length;
>> + const char *security_user;
>> + unsigned int security_user_length;
>> + const char *security_priv_user;
>> + unsigned int security_priv_user_length;
>> + const char *security_ip;
>> + unsigned int security_ip_length;
>> + const char *security_host_or_ip;
>> + unsigned int security_host_or_ip_length;
>> + unsigned long security_global_access;
>> + unsigned long security_schema_access;
>> +
>> + const char *security_type;
>> + unsigned int security_type_length;
>> + const char *security_schema;
>> + unsigned int security_schema_length;
>> + const char *security_object;
>> + unsigned int security_object_length;
>> + const char *security_element;
>> + unsigned int security_element_length;
>> +
>> + unsigned long security_object_rights;
>> +END_DECLARE_AUDIT_EVENT_DATA;
>> +
>> +
>> +/
>> *************************************************************************
>> + Here we define the descriptor structure, that is referred from
>> + st_mysql_plugin.
>> +*/
>> +
>> +struct st_mysql_audit
>> +{
>> + int interface_version;
>> + const unsigned long long *class_mask;
>
> what is class_mask ? how is the mask defined ?
bitwise OR of the declared class mask macros.
>
>> + void (*release_thd)(MYSQL_THD);
>> + void (*event_notify)(MYSQL_THD, const struct mysql_event *);
>> +};
>> +
>
> no comments at all in this file. It *must* be commented, documenting
> itself
> exhaustively. See ftparser.h for example
ok. Will add comments.
>
>> +
>> +#endif
>> 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 2007-11-29 14:40:19 -08:00
>> @@ -0,0 +1,142 @@
>> +/* Copyright (C) 2006-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., 51 Franklin St, Fifth Floor, Boston, MA
>> 02110-1301 USA */
>> +
>> +#include <my_global.h>
>> +#include <my_sys.h>
>> +#include <my_atomic.h>
>
> don't use internal MySQL include files in the example plugins
> (instead of my_atomic you can use a mutex or simply allow your
> counter to be approximate, accessing it without a lock -
> it's an example after all)
Ok.
>
>> +#include <mysql/audit.h>
>> +
>> +#if !defined(__attribute__) && (defined(__cplusplus) || !
>> defined(__GNUC__) || __GNUC__ == 2 && __GNUC_MINOR__ < 8)
>> +#define __attribute__(A)
>> +#endif
>> +
>> +static volatile uint32 number_of_calls; /* for SHOW STATUS, see
>> below */
>> +static my_atomic_rwlock_t rwl;
>> +
>> +
>> +/*
>> + Initialize the plugin at server start or plugin installation.
>> +
>> + SYNOPSIS
>> + null_audit_plugin_init()
>> +
>> + DESCRIPTION
>> + Does nothing.
>> +
>> + RETURN VALUE
>> + 0 success
>> + 1 failure (cannot happen)
>> +*/
>> +
>> +static int null_audit_plugin_init(void *arg __attribute__((unused)))
>> +{
>> + my_atomic_rwlock_init(&rwl);
>> + number_of_calls= 0;
>> + return(0);
>> +}
>> +
>> +
>> +/*
>> + Terminate the plugin at server shutdown or plugin deinstallation.
>> +
>> + SYNOPSIS
>> + null_audit_plugin_deinit()
>> + Does nothing.
>> +
>> + RETURN VALUE
>> + 0 success
>> + 1 failure (cannot happen)
>> +
>> +*/
>> +
>> +static int null_audit_plugin_deinit(void *arg
>> __attribute__((unused)))
>> +{
>> + my_atomic_rwlock_destroy(&rwl);
>> + printf("audit_null was invoked %lu times\n",
>> + (ulong) number_of_calls);
>> + return(0);
>> +}
>> +
>> +
>> +/*
>> + Foo
>> +
>> + SYNOPSIS
>> + null_notify()
>> + thd connection context
>> +
>> + DESCRIPTION
>> +*/
>> +
>> +static void null_notify(MYSQL_THD thd __attribute__((unused)),
>> + const struct mysql_event *event
>> + __attribute__((unused)))
>> +{
>> + my_atomic_rwlock_wrlock(&rwl);
>> + my_atomic_add32(&number_of_calls, 1);
>> + my_atomic_rwlock_wrunlock(&rwl);
>> +}
>> +
>> +
>> +static unsigned long long null_audit_mask[]=
>> +{
>> + (unsigned long long) -1
>> +};
>> +
>> +
>> +/*
>> + Plugin type-specific descriptor
>> +*/
>> +
>> +static struct st_mysql_audit null_audit_descriptor=
>> +{
>> + MYSQL_AUDIT_INTERFACE_VERSION, /* interface version */
>> + null_audit_mask, /* class mask */
>> + NULL, /* release_thd function */
>> + null_notify /* notify function */
>> +};
>> +
>> +/*
>> + Plugin status variables for SHOW STATUS
>> +*/
>> +
>> +static struct st_mysql_show_var simple_status[]=
>> +{
>> + {"null_audit_called", (char *)&number_of_calls, SHOW_INT},
>> + {0,0,0}
>> +};
>> +
>> +
>> +/*
>> + Plugin library descriptor
>> +*/
>> +
>> +mysql_declare_plugin(null_audit)
>> +{
>> + MYSQL_AUDIT_PLUGIN, /* type */
>> + &null_audit_descriptor, /* descriptor */
>> + "NULL_AUDIT", /* name */
>> + "MySQL AB", /* author */
>> + "Simple NULL Audit", /* description */
>> + PLUGIN_LICENSE_GPL,
>> + null_audit_plugin_init, /* init function (when loaded) */
>> + null_audit_plugin_deinit, /* deinit function (when unloaded) */
>> + 0x0001, /* version */
>> + simple_status, /* status variables */
>> + NULL, /* system variables */
>> + NULL
>> +}
>> +mysql_declare_plugin_end;
>> +
>> 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 2007-11-29 14:40:18 -08:00
>> @@ -0,0 +1,95 @@
>> +/* 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 */
>> +
>> +
>> +#ifndef EMBEDDED_LIBRARY
>> +
>> +#include <mysql/audit.h>
>> +
>> +
>> +extern uint64 mysql_global_audit_mask1;
>
> there're no comments in this file either
>
>> +
>> +
>> +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,
>> + int error_code, ...);
>
> remove error_code (it should be in the varargs part)
Ok.
>
>> +extern void mysql_audit_release(THD *thd);
>> +
>> +
>> +
>> +
>
> please remove end-of-line blanks and expands tabs in all new files.
Ok.
>
>> +#define MYSQL_AUDIT_GENERAL(thd,event_subtype,rc,time, \
>> + user,userlen,cmd,cmdlen, \
>> + query,querylen,clientcs,rows) do { \
>> + if (mysql_global_audit_mask1 & MYSQL_AUDIT_GENERAL_CLASSMASK1)\
>> + { \
>> + time_t audit_time= (time); \
>> + const char *audit_user= (user), *audit_cmd= (cmd); \
>> + const char *audit_query= (query); \
>> + uint audit_user_len= (userlen), audit_cmd_len= (cmdlen); \
>> + uint audit_query_len= (querylen); \
>> + CHARSET_INFO *audit_client_cs= (clientcs); \
>> + ha_rows audit_rows=(rows); \
>> + mysql_audit_notify((thd), MYSQL_AUDIT_GENERAL_CLASS, \
>> + (event_subtype), (rc), audit_time, \
>> + audit_user, audit_user_len, \
>> + audit_cmd, audit_cmd_len, \
>> + audit_query, audit_query_len, \
>> + audit_client_cs,audit_rows); \
>> + } \
>> +} while (0)
>
> it's C++, use inline functions instead of complex macros
Ok,
>
>> +
>> +
>> +
>> +#define MYSQL_AUDIT_SECURITY(thd,event_subtype,rc,type,schema, \
>> + object,element,rights) do { \
>> + if (mysql_global_audit_mask1 & MYSQL_AUDIT_SECURITY_CLASSMASK1)\
>> + { \
>> + LEX_STRING audit_object_type= {C_STRING_WITH_LEN(type)}; \
>> + LEX_STRING *audit_schema= (schema); \
>> + LEX_STRING *audit_object= (object); \
>> + LEX_STRING *audit_element= (element); \
>> + ulong audit_rights= (rights); \
>> + mysql_audit_notify((thd), MYSQL_AUDIT_SECURITY_CLASS, \
>> + (event_subtype),(rc),&audit_object_type, \
>> + &audit_schema,&audit_object, \
>> + &audit_element,audit_rights); \
>> + } \
>> +} while (0)
>> +
>> +#else /* EMBEDDED_LIBRARY */
>> +
>> +/* In EMBEDDED_LIBRARY builds, these do nothing */
>> +
>> +#define MYSQL_AUDIT_GENERAL(thd,event_subtype,rc,time, \
>> + user,userlen,cmd,cmdlen, \
>> + query,querylen,clientcs,rows) do { \
>> +} while (0)
>> +#define MYSQL_AUDIT_SECURITY(thd,event_subtype,rc,type,schema, \
>> + object,element,rights) do { \
>> +} while (0)
>> +
>> +#define mysql_audit_release(thd) do { } while (0)
>> +
>> +
>> +#endif /* EMBEDDED_LIBRARY */
>> 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 2007-11-29 14:40:18 -08:00
>> @@ -0,0 +1,502 @@
>> +/* 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_priv.h"
>> +#include "sql_audit.h"
>> +
>> +extern int initialize_audit_plugin(st_plugin_int *plugin);
>> +extern int finalize_audit_plugin(st_plugin_int *plugin);
>> +
>> +#ifndef EMBEDDED_LIBRARY
>> +
>> +uint64 mysql_global_audit_mask1= 0;
>> +
>> +
>> +static void event_class_dispatch(THD *thd, const struct
>> mysql_event *event);
>> +
>> +typedef void (*audit_handler_t)(THD *thd, uint event_subtype,
>> + int error_code, va_list ap);
>> +
>> +/**
>> + MYSQL_AUDIT_GENERAL_CLASS handler
>> +
>> + @param[in] thd
>> + @param[in] event_subtype
>> + @param[in] error_code
>> + @param[in] ap
>> +
>> +*/
>> +
>> +static void general_class_handler(THD *thd, uint event_subtype,
>> + int error_code, va_list ap)
>> +{
>> + mysql_event_general event;
>> + event.event_class= MYSQL_AUDIT_GENERAL_CLASS;
>> + event.general_error_code= error_code;
>> + event.general_thread_id= thd ? thd->thread_id : 0;
>> + event.general_time= va_arg(ap, time_t);
>> + event.general_user= va_arg(ap, const char *);
>> + event.general_user_length= va_arg(ap, unsigned int);
>> + event.general_command= va_arg(ap, const char *);
>> + event.general_command_length= va_arg(ap, unsigned int);
>> + event.general_query= va_arg(ap, const char *);
>> + event.general_query_length= va_arg(ap, unsigned int);
>> + event.general_charset= va_arg(ap, struct charset_info_st *);
>> + event.general_rows= (unsigned long long) va_arg(ap, ha_rows);
>> + event_class_dispatch(thd, &event);
>> +}
>> +
>> +
>> +/**
>> + MYSQL_AUDIT_SECURITY_CLASS handler
>> +
>> + @param[in] thd
>> + @param[in] event_subtype
>> + @param[in] error_code
>> + @param[in] ap
>> +
>> +*/
>> +
>> +static void security_class_handler(THD *thd, uint event_subtype,
>> + int error_code, va_list ap)
>> +{
>> + mysql_event_security event;
>> + LEX_STRING *str;
>> +
>> + bzero(&event, sizeof(event));
>> + event.event_class= MYSQL_AUDIT_SECURITY_CLASS;
>> + event.security_error_code= error_code;
>> +
>> + if (thd)
>> + {
>> + event.security_thread_id= thd->thread_id;
>> + Security_context *ctx= thd->security_ctx;
>> + event.security_host= ctx->host;
>> + event.security_host_length=
>> + event.security_host ? strlen(event.security_host) : 0;
>> + event.security_user= ctx->user;
>> + event.security_user_length=
>> + event.security_user ? strlen(event.security_user) : 0;
>> + event.security_priv_user= ctx->priv_user;
>> + event.security_priv_user_length=
>> + event.security_priv_user ?
>> strlen(event.security_priv_user) : 0;
>> + event.security_ip= ctx->ip;
>> + event.security_ip_length=
>> + event.security_ip ? strlen(event.security_ip) : 0;
>> + event.security_host_or_ip= ctx->host_or_ip;
>> + event.security_host_or_ip_length=
>> + event.security_host_or_ip ?
>> strlen(event.security_host_or_ip) : 0;
>> + event.security_global_access= ctx->master_access;
>> + event.security_schema_access= ctx->db_access;
>> + }
>> +
>> + if ((str= va_arg(ap, LEX_STRING *)))
>> + {
>> + event.security_type= str->str;
>> + event.security_type_length= str->length;
>> + }
>> + if ((str= va_arg(ap, LEX_STRING *)))
>> + {
>> + event.security_schema= str->str;
>> + event.security_schema_length= str->length;
>> + }
>> + if ((str= va_arg(ap, LEX_STRING *)))
>> + {
>> + event.security_object= str->str;
>> + event.security_object_length= str->length;
>> + }
>> + if ((str= va_arg(ap, LEX_STRING *)))
>> + {
>> + event.security_element= str->str;
>> + event.security_element_length= str->length;
>> + }
>> + event.security_object_rights= va_arg(ap, ulong);
>> +
>> + event_class_dispatch(thd, &event);
>> +}
>> +
>> +
>> +static audit_handler_t audit_handlers[] =
>> +{
>> + /* MYSQL_AUDIT_GENERAL_CLASS */ general_class_handler,
>> + /* MYSQL_AUDIT_SECURITY_CLASS */ security_class_handler,
>
> these two comments inside audit_handlers[] are redundant
> it's like
> i++; // increment i
I originally had less informative function names - I will correct this.
>
>> + NULL
>
> it doesn't have to be NULL-terminated. Furthermore it's confusing and
> wrong to make it NULL-terminated
Laziness on my part... Originally it was null terminated and I haven't
removed it.
>
>
>> +};
>> +
>> +static const uint audit_handlers_count=
>> + (sizeof(audit_handlers) / sizeof(audit_handler_t)) - 1;
>> +
>> +
>> +/**
>> + Acquire and lock any additional audit plugins as required
>> +
>> + @param[in] thd
>> + @param[in] plugin
>> + @param[in] arg
>> +
>> + @retval FALSE Always
>> +*/
>> +
>> +static my_bool acquire_plugins(THD *thd, plugin_ref plugin, void
>> *arg)
>> +{
>> + uint event_class= *(uint*) arg;
>> + uint64 event_class_mask1= (1 << event_class);
>> + st_mysql_audit *data= plugin_data(plugin, struct st_mysql_audit
>> *);
>> +
>> + /* Check if this plugin is interested in the event */
>> + if (!(data->class_mask[0] & event_class_mask1))
>> + return 0;
>
> So, you preferred to scan the list of plugins...
> (in the schema that I suggested it would be not necessary)
> okay, I think it shouldn't be a problem for a resonable (read: small)
> number of plugins. I don't see us having a couple of thousands
> of active audit plugins yet.
>
>> +
>> + /* Check if we need to initialize the array of acquired plugins */
>> + if (unlikely(!thd->audit_class_plugins.buffer))
>> + {
>> + /* specify some reasonable initialization defaults */
>> + my_init_dynamic_array(&thd->audit_class_plugins,
>> + sizeof(plugin_ref), 16, 16);
>> + }
>> + else
>> + {
>> + plugin_ref *plugins, *plugins_last;
>> +
>> + plugins= (plugin_ref*) thd->audit_class_plugins.buffer;
>> + plugins_last= plugins + thd->audit_class_plugins.elements;
>> + for (; plugins < plugins_last; plugins++)
>> + {
>> + st_mysql_audit *data2= plugin_data(*plugins, struct
>> st_mysql_audit *);
>> +
>> + /* Check if we already have acquired this plugin */
>> + if (data == data2)
>> + return 0;
>> + }
>
> you don't need this loop. a plugin can be already acquired only if
> this plugin has registered for many audit classes, and it was acquired
> for a different audit class. Thus, instead of the loop you can write
>
> if (thd->audit_class_mask1 & data->class_mask[0]) // already acquired
> return 0;
>
> there's a boundary case when a plugin was installed after thd has
> registered
> plugins for this class, as in
> plugin1 (classA) is installed
> thd emits event of the classA
> thd acquires plugin1, adds classA to audit_class_mask1
> plugin2 (classA + classB) is installed
> thd emits event of the classB
> in this case my if() will skip plugin2. I believe we can ignore this
> special
> case and optimize for all plugins being installed.
Ok, I will alter the implementation and document the special case.
>
>> + }
>> +
>> + /* lock the plugin and add it to the list */
>> + plugin= my_plugin_lock(NULL, &plugin);
>> + insert_dynamic(&thd->audit_class_plugins, (uchar*) &plugin);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/**
>> + Notify the audit system of an event
>> +
>> + @param[in] thd
>> + @param[in] event_class
>> + @param[in] event_subtype
>> + @param[in] error_code
>> +
>> +*/
>> +
>> +void mysql_audit_notify(THD *thd, uint event_class, uint
>> event_subtype,
>> + int error_code, ...)
>> +{
>> + va_list ap;
>> + audit_handler_t *handlers= audit_handlers + event_class;
>> + uint64 event_class_mask1= (1 << event_class);
>> +
>> + DBUG_ASSERT(event_class < audit_handlers_count);
>> +
>> + if (thd && thd->killed == THD::KILL_CONNECTION &&
>> + !(thd->audit_class_mask1 & event_class_mask1))
>> + thd= NULL;
>
> why ? (add a comment)
Ok,
>
>> +
>> + /*
>> + Check to see if we have acquired the audit plugins for the
>> + required audit event classes.
>> + */
>> + if (thd && !(thd->audit_class_mask1 & event_class_mask1))
>> + {
>> + plugin_foreach(thd, acquire_plugins, MYSQL_AUDIT_PLUGIN,
>> &event_class);
>> + thd->audit_class_mask1|= event_class_mask1;
>> + }
>> +
>> + va_start(ap, error_code);
>> + (*handlers)(thd, event_subtype, error_code, ap);
>> + va_end(ap);
>> +}
>> +
>> +
>> +/**
>> + Release any resources associated with the current thd.
>> +
>> + @param[in] thd
>> +
>> +*/
>> +
>> +void mysql_audit_release(THD *thd)
>> +{
>> + plugin_ref *plugins, *plugins_last;
>> +
>> + if (!thd || !(thd->audit_class_plugins.elements))
>> + return;
>> +
>> + plugins= (plugin_ref*) thd->audit_class_plugins.buffer;
>> + plugins_last= plugins + thd->audit_class_plugins.elements;
>> + for (; plugins < plugins_last; plugins++)
>> + {
>> + st_mysql_audit *data= plugin_data(*plugins, struct
>> st_mysql_audit *);
>> +
>> + /* Check to see if the plugin has a release method */
>> + if (!(data->release_thd))
>> + continue;
>> +
>> + /* Tell the plugin to release its resources */
>> + data->release_thd(thd);
>> + }
>> +
>> + /* Now we actually unlock the plugins */
>> + plugin_unlock_list(NULL, (plugin_ref*) thd-
>> >audit_class_plugins.buffer,
>> + thd->audit_class_plugins.elements);
>> +
>> + /* Reset the state of thread values */
>> + reset_dynamic(&thd->audit_class_plugins);
>> + thd->audit_class_mask1= 0;
>> +}
>> +
>> +
>> +/**
>> + Initialize thd variables used by Audit
>> +
>> + @param[in] thd
>> +
>> +*/
>> +
>> +void mysql_audit_init_thd(THD *thd)
>> +{
>> + bzero(&thd->audit_class_plugins, sizeof(thd-
>> >audit_class_plugins));
>> + thd->audit_class_mask1= 0;
>> +}
>> +
>> +
>> +/**
>> + Free thd variables used by Audit
>> +
>> + @param[in] thd
>> + @param[in] plugin
>> + @param[in] arg
>> +
>> + @retval FALSE Always
>> +*/
>> +
>> +void mysql_audit_free_thd(THD *thd)
>> +{
>> + mysql_audit_release(thd);
>> + DBUG_ASSERT(thd->audit_class_plugins.elements == 0);
>> + delete_dynamic(&thd->audit_class_plugins);
>> +}
>> +
>> +
>> +static DYNAMIC_ARRAY installed_audit_plugins;
>> +static pthread_mutex_t LOCK_audit;
>
> why do you need a second list of all audit plugins ?
> it'll be a copy of duplicate plugin_hash[MYSQL_AUDIT_PLUGIN].records
> (which you can iterate with plugin_foreach(), as you know)
When sending event notifications when thd==NULL, I did not want to
rely on obtaining plugin system lock in case where a specific plugin
has created its own threads and performed some operation which would
result in deadlock. So implementation is declared so that
>
>> +
>> +
>> +/**
>> + Initialize Audit global variables
>> +*/
>> +
>> +void mysql_audit_initialize()
>> +{
>> + (void) pthread_mutex_init(&LOCK_audit, MY_MUTEX_INIT_FAST);
>> + my_init_dynamic_array(&installed_audit_plugins, sizeof(void*),
>> 16, 16);
>> +}
>> +
>> +
>> +/**
>> + Finalize Audit global variables
>> +*/
>> +
>> +void mysql_audit_finalize()
>> +{
>> + delete_dynamic(&installed_audit_plugins);
>> + (void) pthread_mutex_destroy(&LOCK_audit);
>> +}
>> +
>> +
>> +/**
>> + Initialize an Audit plug-in
>> +
>> + @param[in] plugin
>> +
>> + @retval FALSE OK
>> + @retval TRUE There was an error.
>> +*/
>> +
>> +int initialize_audit_plugin(st_plugin_int *plugin)
>> +{
>> + st_mysql_audit *data= (st_mysql_audit*) plugin->plugin->info;
>> + int rc;
>> +
>> + if (!data->class_mask || !data->event_notify ||
>> + !data->class_mask[0])
>> + {
>> + sql_print_error("Plugin '%s' has invalid data.",
>> + plugin->name.str);
>> + return 1;
>> + }
>> +
>> + if (plugin->plugin->init && plugin->plugin->init(NULL))
>> + {
>> + sql_print_error("Plugin '%s' init function returned error.",
>> + plugin->name.str);
>> + return 1;
>> + }
>> +
>> + /* Make the interface info more easily accessible */
>> + plugin->data= plugin->plugin->info;
>> +
>> + pthread_mutex_lock(&LOCK_audit);
>> +
>> + rc= insert_dynamic(&installed_audit_plugins, (uchar*) &plugin-
>> >data);
>> +
>> + /* Add the bits the plugin is interested in to the global mask */
>> + mysql_global_audit_mask1|= data->class_mask[0];
>> +
>> + pthread_mutex_unlock(&LOCK_audit);
>> +
>> + return rc;
>> +}
>> +
>> +
>> +/**
>> + Finalize an Audit plug-in
>> +
>> + @param[in] plugin
>> +
>> + @retval FALSE OK
>> + @retval TRUE There was an error.
>> +*/
>> +int finalize_audit_plugin(st_plugin_int *plugin)
>> +{
>> + uint64 event_class_mask1= 0;
>> + st_mysql_audit **data, **data_last;
>> + uint index, index_to_delete= (uint) -1;
>> +
>> + if (plugin->plugin->deinit &&
> plugin->plugin->deinit(NULL))
>> + {
>> + DBUG_PRINT("warning", ("Plugin '%s' deinit function returned
>> error.",
>> + plugin->name.str));
>> + DBUG_EXECUTE("finalize_audit_plugin", return 1; );
>> + }
>> +
>> + pthread_mutex_lock(&LOCK_audit);
>> +
>> + /* Iterate through all the installed plugins to create new mask */
>> + data= (st_mysql_audit**) installed_audit_plugins.buffer;
>> + data_last= data + installed_audit_plugins.elements;
>> + for (index= 0; data < data_last; data++, index++)
>> + {
>> + if (plugin->data == *data)
>> + index_to_delete= index;
>> + else
>> + event_class_mask1|= (*data)->class_mask[0];
>> + }
>> +
>> + /* Set the global audit mask */
>> + mysql_global_audit_mask1= event_class_mask1;
>> +
>> + /* Remove this plugin from the list */
>> + if (likely(index_to_delete != (uint) -1))
>> + delete_dynamic_element(&installed_audit_plugins,
>> index_to_delete);
>> + else
>> + {
>> + sql_print_error("Plugin '%s' was not found in
>> installed_audit_plugins",
>> + plugin->name.str);
>> + }
>> +
>> + pthread_mutex_unlock(&LOCK_audit);
>> +
>> + return 0;
>> +}
>> +
>> +
>> +/**
>> + Distributes an audit event to plug-ins
>> +
>> + @param[in] thd
>> + @param[in] event
>> +*/
>> +
>> +static void event_class_dispatch(THD *thd, const struct
>> mysql_event *event)
>> +{
>> + uint64 event_class_mask1= (1 << event->event_class);
>> +
>> + if (thd)
>> + {
>> + plugin_ref *plugins, *plugins_last;
>> +
>> + /* Use the cached set of audit plugins */
>> + plugins= (plugin_ref*) thd->audit_class_plugins.buffer;
>> + plugins_last= plugins + thd->audit_class_plugins.elements;
>> + for (; plugins < plugins_last; plugins++)
>> + {
>> + st_mysql_audit *data= plugin_data(*plugins, struct
>> st_mysql_audit *);
>> +
>> + /* Check to see if the plugin is interested in this event */
>> + if (!(data->class_mask[0] & event_class_mask1))
>> + continue;
>> +
>> + /* Actually notify the plugin */
>> + data->event_notify(thd, event);
>> + }
>> + }
>> + else
>> + {
>> + st_mysql_audit **data, **data_last;
>> +
>> + pthread_mutex_lock(&LOCK_audit);
>> +
>> + /* Use our own list of all audit plugins */
>> + data= (st_mysql_audit**) installed_audit_plugins.buffer;
>> + data_last= data + installed_audit_plugins.elements;
>> + for (; data < data_last; data++)
>> + {
>> + if (!((*data)->class_mask[0] & event_class_mask1))
>> + continue;
>> +
>> + /* Actually notify the plugin */
>> + (*data)->event_notify(0, event);
>> + }
>> +
>> + pthread_mutex_unlock(&LOCK_audit);
>> + }
>> +}
>> +
>> +
>> +
>> +
>> +#else /* EMBEDDED_LIBRARY */
>> +
>> +void mysql_audit_initialize()
>> +{
>> +}
>> +
>> +
>> +void mysql_audit_finalize()
>> +{
>> +}
>> +
>> +
>> +int initialize_audit_plugin(st_plugin_int *plugin)
>> +{
>> + return 1;
>> +}
>> +
>> +int finalize_audit_plugin(st_plugin_int *plugin)
>> +{
>> + return 0;
>> +}
>> +
>> +#endif /* EMBEDDED_LIBRARY */
>> diff -Nrup a/sql/sql_plugin.cc b/sql/sql_plugin.cc
>> --- a/sql/sql_plugin.cc 2007-11-21 12:19:04 -08:00
>> +++ b/sql/sql_plugin.cc 2007-11-29 14:40:18 -08:00
>> @@ -174,7 +186,8 @@ public:
>> sys_var_pluginvar *cast_pluginvar() { return this; }
>> bool is_readonly() const { return plugin_var->flags &
>> PLUGIN_VAR_READONLY; }
>> bool check_type(enum_var_type type)
>> - { return !(plugin_var->flags & PLUGIN_VAR_THDLOCAL) && type !=
>
>> OPT_GLOBAL; }
>> + { return !(plugin_var->flags & PLUGIN_VAR_THDLOCAL) ? (type !=
>> OPT_GLOBAL)
>> + : ((type == OPT_GLOBAL) && (plugin_var->flags &
>> PLUGIN_VAR_NOGLOBAL)); }
>
> no PLUGIN_VAR_NOGLOBAL
ok
>
>> bool check_update_type(Item_result type);
>> SHOW_TYPE show_type();
>> uchar* real_value_ptr(THD *thd, enum_var_type type);
>> @@ -330,6 +343,11 @@ static inline void free_plugin_mem(struc
>> my_free((uchar*)p->plugins, MYF(MY_ALLOW_ZERO_PTR));
>> }
>>
>> +extern "C" char *mysqld_plugin_path(char *tgt, unsigned int
>> tgt_size,
>> + const char *name)
>> +{
>> + return strxnmov(tgt, tgt_size - 1, opt_plugin_dir, "/", name,
>> NullS);
>> +}
>
> What is that ?
>
>> static st_plugin_dl *plugin_dl_add(const LEX_STRING *dl, int report)
>> {
>> @@ -346,7 +364,8 @@ static st_plugin_dl *plugin_dl_add(const
>> plugin directory are used (to make this even remotely secure).
>> */
>> if (my_strchr(files_charset_info, dl->str, dl->str + dl->length,
>> FN_LIBCHAR) ||
>> - check_identifier_name((LEX_STRING *) dl) ||
>> + check_string_char_length((LEX_STRING *) dl, "", NAME_CHAR_LEN,
>
> indeed
>
>> + system_charset_info, 1) ||
>> plugin_dir_len + dl->length + 1 >= FN_REFLEN)
>> {
>> if (report & REPORT_TO_USER)
>> @@ -1179,9 +1198,7 @@ int plugin_init(int *argc, char **argv,
>> /* Register all dynamic plugins */
>> if (!(flags & PLUGIN_INIT_SKIP_DYNAMIC_LOADING))
>> {
>> - if (opt_plugin_load &&
>> - plugin_load_list(&tmp_root, argc, argv, opt_plugin_load))
>> - goto err;
>> + plugin_load_list(&tmp_root, argc, argv, opt_plugin_load);
>
> what was wrong with plugin_load_list ?
I did not want failure to load a plugin to cause the mysqld to
terminate.
>
>> if (!(flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE))
>> plugin_load(&tmp_root, argc, argv);
>> }
>> @@ -1310,27 +1327,23 @@ end:
>> */
>> static void plugin_load(MEM_ROOT *tmp_root, int *argc, char **argv)
>> {
>> + THD thd;
>> TABLE_LIST tables;
>> TABLE *table;
>> READ_RECORD read_record_info;
>> int error;
>> - THD *new_thd;
>> + THD *new_thd= &thd;
>
> why ?
did not seem to be much value declaring this temporary THD on the heap.
>
>> #ifdef EMBEDDED_LIBRARY
>> bool table_exists;
>> #endif /* EMBEDDED_LIBRARY */
>> DBUG_ENTER("plugin_load");
>>
>> - if (!(new_thd= new THD))
>> - {
>> - sql_print_error("Can't allocate memory for plugin structures");
>> - delete new_thd;
>> - DBUG_VOID_RETURN;
>> - }
>> new_thd->thread_stack= (char*) &tables;
>> new_thd->store_globals();
>> lex_start(new_thd);
>> new_thd->db= my_strdup("mysql", MYF(0));
>> new_thd->db_length= 5;
>> + bzero((char*) &thd.net, sizeof(thd.net));
>> bzero((uchar*)&tables, sizeof(tables));
>> tables.alias= tables.table_name= (char*)"plugin";
>> tables.lock_type= TL_READ;
>> @@ -1406,11 +1418,15 @@ static bool plugin_load_list(MEM_ROOT *t
>> struct st_plugin_dl *plugin_dl;
>
> by the way, please remove argument names from the forward
> declaration of
> plugin_load_list:
>
> -static bool plugin_load_list(MEM_ROOT *tmp_root, int *argc, char
> **argv,
> - const char *list);
> +static bool plugin_load_list(MEM_ROOT *, int *, char **, const char
> *);
ok
>
>> struct st_mysql_plugin *plugin;
>> char *p= buffer;
>> + bool result= FALSE;
>> DBUG_ENTER("plugin_load_list");
>> while (list)
>> {
>> if (p == buffer + sizeof(buffer) - 1)
>> + {
>> + sql_print_warning("plugin-load argument too long");
>> break;
>> + }
>> switch ((*(p++)= *(list++))) {
>> case '\0':
>> list= NULL; /* terminate the loop */
>> @@ -1420,6 +1436,7 @@ static bool plugin_load_list(MEM_ROOT *t
>> #endif
>> case ';':
>> name.str[name.length]= '\0';
>> + pthread_mutex_lock(&LOCK_plugin);
>
> why ? safe_mutex_assert_owner()'s ?
> why not to lock once in the beginning on the function and unlock on
> return ?
> some other code that expects the mutex to be NOT locked ?
I will alter accordingly.
>
>> if (str != &dl) // load all plugins in named module
>> {
>> dl= name;
>> @@ -1459,12 +1483,12 @@ static bool plugin_load_list(MEM_ROOT *t
>> str->length++;
>> continue;
>> }
>> - }
>> - DBUG_RETURN(FALSE);
>> error:
>> - sql_print_error("Couldn't load plugin named '%s' with soname
>> '%s'.",
>> - name.str, dl.str);
>> - DBUG_RETURN(TRUE);
>> + sql_print_warning("Couldn't load plugin named '%s' with soname
>> '%s'.",
>> + name.str, dl.str);
>> + result= TRUE;
>> + }
>> + DBUG_RETURN(result);
>
> why do you return a value if you ignore it in the caller ?
I am undecided if a failure to load a plugin is reasonable grounds to
terminate the server. I would appreciate your input on that decision.
>
>> }
>>
>>
>> @@ -1625,16 +1649,28 @@ bool mysql_install_plugin(THD *thd, cons
>> DBUG_RETURN(TRUE);
>>
>> pthread_mutex_lock(&LOCK_plugin);
>> - rw_wrlock(&LOCK_system_variables_hash);
>> +
>> + table->use_all_columns();
>> + restore_record(table, s->default_values);
>> + table->field[0]->store(name->str, name->length,
>> system_charset_info);
>> + table->field[1]->store(dl->str, dl->length, files_charset_info);
>> + error= table->file->ha_write_row(table->record[0]);
>> + if (error)
>> + {
>> + table->file->print_error(error, MYF(0));
>> + goto err;
>> + }
>
> why ?
This, I need to try to remember as I vaguely recall a deadlock/crash
scenario which resulted in these changes in sql_plugin.cc
>
>> /* handle_options() assumes arg0 (program name) always exists */
>> argv[0]= const_cast<char*>(""); // without a cast gcc emits a
>> warning
>> argv[1]= 0;
>> argc= 1;
>> + rw_wrlock(&LOCK_system_variables_hash);
>> error= plugin_add(thd->mem_root, name, dl, &argc, argv,
>> REPORT_TO_USER);
>> rw_unlock(&LOCK_system_variables_hash);
>>
>> if (error || !(tmp= plugin_find_internal(name, MYSQL_ANY_PLUGIN)))
>> - goto err;
>> + goto deinit;
>>
>> if (plugin_initialize(tmp))
>> {
>>
Regards,
Antony
