MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Sergei Golubchik Date:February 27 2008 8:52pm
Subject:Re: bk commit into 6.0 tree (antony:1.2573) WL#3771
View as plain text  
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
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