maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

ok to push. A few minor questions inline.

On Mon, Jun 16, 2014 at 09:34:21AM +0200, serg@xxxxxxxxxxx wrote:
> revision-id: 7d386bbce216745ba28df728f45a0b796fcced98
> parent(s): f61f36b386e8d0c6448297bb84c92e8d9b82be6a
> committer: Sergei Golubchik
> branch nick: maria
> timestamp: 2014-06-15 17:12:57 +0200
> message:
> 
> MDEV-4549 [PATCH] Clean up code working with ACL tables
> 
> enum values to index different ACL tables, instead of hard-coded numbers
> (even different in diffent functions). Also factor out TABLE_LIST
> initialization into a separate function.
> 
> ---
>  mysql-test/r/not_embedded_server.result            |   4 +-
>  mysql-test/r/sp-destruct.result                    |   3 -
>  .../suite/rpl/r/rpl_tmp_table_and_DDL.result       |  22 +-
>  mysql-test/t/not_embedded_server.test              |   2 +-
>  mysql-test/t/sp-destruct.test                      |   1 -
>  sql/sql_acl.cc                                     | 533 +++++++++------------
>  6 files changed, 235 insertions(+), 330 deletions(-)
> 
> diff --git a/mysql-test/r/not_embedded_server.result b/mysql-test/r/not_embedded_server.result
> index 2295276..6dda855 100644
> --- a/mysql-test/r/not_embedded_server.result
> +++ b/mysql-test/r/not_embedded_server.result
> @@ -1,4 +1,4 @@
> -call mtr.add_suppression("Can't open and lock privilege tables: Table 'host' was not locked with LOCK TABLES");
> +call mtr.add_suppression("Can't open and lock privilege tables: Table 'roles_mapping' was not locked with LOCK TABLES");
>  SHOW VARIABLES like 'slave_skip_errors';
>  Variable_name	Value
>  slave_skip_errors	OFF
Why did it change (here and in other tests)? I didn't notice any table order
change in the code.

...skip...

> diff --git a/mysql-test/r/sp-destruct.result b/mysql-test/r/sp-destruct.result
> index 172e40c..fe68229 100644
> --- a/mysql-test/r/sp-destruct.result
> +++ b/mysql-test/r/sp-destruct.result
> @@ -128,11 +128,8 @@ CREATE FUNCTION f1() RETURNS INT RETURN 1;
>  RENAME TABLE mysql.procs_priv TO procs_priv_backup;
>  FLUSH TABLE mysql.procs_priv;
>  DROP FUNCTION f1;
> -ERROR 42S02: Table 'mysql.procs_priv' doesn't exist
>  SHOW WARNINGS;
>  Level	Code	Message
> -Error	1146	Table 'mysql.procs_priv' doesn't exist
> -Warning	1405	Failed to revoke all privileges to dropped routine
>  # Restore the procs_priv table
>  RENAME TABLE procs_priv_backup TO mysql.procs_priv;
>  FLUSH TABLE mysql.procs_priv;
Hmm... why?

...skip...

> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index 5e8e0e7..442d512 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -380,7 +380,7 @@ class ACL_PROXY_USER :public ACL_ACCESS
>      MYSQL_PROXIES_PRIV_PROXIED_USER,
>      MYSQL_PROXIES_PRIV_WITH_GRANT,
>      MYSQL_PROXIES_PRIV_GRANTOR,
> -    MYSQL_PROXIES_PRIV_TIMESTAMP } old_acl_proxy_users;
> +    MYSQL_PROXIES_PRIV_TIMESTAMP } proxy_table_fields;
>  public:
>    ACL_PROXY_USER () {};
>  
> @@ -760,6 +760,99 @@ static int traverse_role_graph_down(ACL_USER_BASE *, void *,
>                               int (*) (ACL_USER_BASE *, ACL_ROLE *, void *));
>  
>  /*
> + Enumeration of ACL/GRANT tables in the mysql database
> +*/
> +enum enum_acl_tables
> +{
> +  USER_TABLE,
> +  DB_TABLE,
> +  TABLES_PRIV_TABLE,
> +  COLUMNS_PRIV_TABLE,
> +#define FIRST_OPTIONAL_TABLE HOST_TABLE
> +  HOST_TABLE,
> +  PROCS_PRIV_TABLE,
> +  PROXIES_PRIV_TABLE,
> +  ROLES_MAPPING_TABLE,
> +  TABLES_MAX // <== always the last
> +};
> +// bits for init_priv_tables
> +static const int Table_user= 1 << USER_TABLE;
> +static const int Table_db= 1 << DB_TABLE;
> +static const int Table_tables_priv= 1 << TABLES_PRIV_TABLE;
> +static const int Table_columns_priv= 1 << COLUMNS_PRIV_TABLE;
> +static const int Table_host= 1 << HOST_TABLE;
> +static const int Table_procs_priv= 1 << PROCS_PRIV_TABLE;
> +static const int Table_proxies_priv= 1 << PROXIES_PRIV_TABLE;
> +static const int Table_roles_mapping= 1 << ROLES_MAPPING_TABLE;
> +
> +const LEX_STRING acl_table_names[]=     //  matches enum_acl_tables
> +{
> +  { C_STRING_WITH_LEN("user") },
> +  { C_STRING_WITH_LEN("db") },
> +  { C_STRING_WITH_LEN("tables_priv") },
> +  { C_STRING_WITH_LEN("columns_priv") },
> +  { C_STRING_WITH_LEN("host") },
> +  { C_STRING_WITH_LEN("procs_priv") },
> +  { C_STRING_WITH_LEN("proxies_priv") },
> +  { C_STRING_WITH_LEN("roles_mapping") }
> +};
> +
> +/**
> +  Initialize a TABLE_LIST array to a set of acl/grant tables.
> +  All tables will be opened with the same lock type, either read or write.
> +
> +  @retval !0 a pointer to the first initialized element in the TABLE_LIST
> +  @retval  0 replication filters matched. Abort operation, but return OK (!)
> +*/
> +TABLE_LIST *init_priv_tables(THD *thd, TABLE_LIST *tables,
> +                             enum thr_lock_type lock_type, int tables_to_open)
Nice clean-up, I assume there was a reason not rewrite open_grant_tables()
instead.

> +{
> +  int prev= -1;
> +  bzero(tables, sizeof(TABLE_LIST) * TABLES_MAX);
> +  for (int cur=0, mask=1; mask <= tables_to_open; cur++, mask <<= 1)
> +  {
> +    if ((tables_to_open & mask) == 0)
> +      continue;
> +    tables[cur].init_one_table(C_STRING_WITH_LEN("mysql"),
> +                               acl_table_names[cur].str,
> +                               acl_table_names[cur].length,
> +                               acl_table_names[cur].str, lock_type);
> +    tables[cur].open_type= OT_BASE_ONLY;
> +    if (lock_type >= TL_WRITE_ALLOW_WRITE)
> +      tables[cur].updating= 1;
> +    if (cur >= FIRST_OPTIONAL_TABLE)
> +      tables[cur].open_strategy= TABLE_LIST::OPEN_IF_EXISTS;
> +    if (prev != -1)
> +      tables[cur].next_local= tables[cur].next_global= & tables[prev];
> +    prev= cur;
> +  }
> +
> +#ifdef HAVE_REPLICATION
> +  if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont)
> +  {
> +    /*
> +      GRANT and REVOKE are applied the slave in/exclusion rules as they are
> +      some kind of updates to the mysql.% tables.
> +    */
> +    Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter;
> +    if (rpl_filter->is_on() && !rpl_filter->tables_ok(0, tables))
> +      return NULL;
> +  }
> +#endif
Was it a bug that mysql_grant_role() didn't have this code? 
open_grant_tables() does updating= 0 after tables_ok(). Is that needed?

...skip...

> @@ -5783,8 +5796,9 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc,
>        }
>      }
>  
> -    if (replace_routine_table(thd, grant_name, tables[1].table, *Str,
> -                              db_name, table_name, is_proc, rights,
> +    if (no_such_table(tables + PROCS_PRIV_TABLE) ||
> +        replace_routine_table(thd, grant_name, tables[PROCS_PRIV_TABLE].table,
> +                              *Str, db_name, table_name, is_proc, rights,
>                                revoke_grant) != 0)
>      {
>        result= TRUE;
Why no_such_table() here?

...skip...

> @@ -6250,7 +6232,7 @@ bool mysql_grant(THD *thd, const char *db, List <LEX_USER> &list,
>        ulong db_rights= rights & DB_ACLS;
>        if (db_rights  == rights)
>        {
> -	if (replace_db_table(tables[1].table, db, *Str, db_rights,
> +	if (replace_db_table(tables[DB_TABLE].table, db, *Str, db_rights,
>  			     revoke_grant))
>  	  result= -1;
>        }
Tabs here (and in a few hunks below).

> @@ -6262,9 +6244,11 @@ bool mysql_grant(THD *thd, const char *db, List <LEX_USER> &list,
>      }
>      else if (is_proxy)
>      {
> -      if (replace_proxies_priv_table (thd, tables[1].table, Str, proxied_user,
> -                                    rights & GRANT_ACL ? TRUE : FALSE,
> -                                    revoke_grant))
> +      if (no_such_table(tables + PROXIES_PRIV_TABLE) ||
> +          replace_proxies_priv_table (thd, tables[PROXIES_PRIV_TABLE].table,
> +                                      Str, proxied_user,
> +                                      rights & GRANT_ACL ? TRUE : FALSE,
> +                                      revoke_grant))
>          result= -1;
>      }
>      if (Str->is_role())
Why no_such_table() here?

...skip...

Regards,
Sergey