MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Antony T Curtis Date:February 29 2008 10:58pm
Subject:Re: bk commit into 6.0 tree (antony:1.2573) WL#3771
View as plain text  
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
>

Thread
bk commit into 6.0 tree (antony:1.2573) WL#3771antony22 Feb
  • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik27 Feb
    • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Antony T Curtis29 Feb
      • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik14 Mar
        • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Antony T Curtis14 Mar
          • Re: bk commit into 6.0 tree (antony:1.2573) WL#3771Sergei Golubchik17 Mar