Hi Sergei, ok to push. A few minor questions inline. On Mon, Jun 16, 2014 at 09:34:21AM +0200, serg@mariadb.org 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