Ingo,
I am implementing the changes. However, I wanted to answer some specific
ones.
>> BUG#44787 : Backup: Check privileges before executing BACKUP/RESTORE
>>
>> Restore can fail in the middle if the user does not have
>> sufficient privileges to create and populate all of the
>> objects.
>>
>> This patch implements a privilege precheck step to check
>> all objects for proper access. If any object fails the
>> privilege check, restore halts with an error.
>>
>
>
> 1) Can you please add a pointer to the documentation, where we explain
> the relation between BACKUP/RESTORE and replication/binlog/SUPER privilege?
From this link:
http://dev.mysql.com/doc/refman/5.4/en/stored-programs-logging.html
We find: "To create or alter a stored function, you must have the SUPER
privilege, in addition to the CREATE ROUTINE or ALTER ROUTINE privilege
that is normally required."
>> +#
>> +
>> +--source include/not_embedded.inc
>> +
>> +disable_query_log;
>> +call mtr.add_suppression("Backup:");
>> +call mtr.add_suppression("Restore:");
>> +enable_query_log;
>
>
> 3) Hema uses to ask for narrower suppressions. I concur.
Sorry. Can you explain? Do you mean suppression for specific messages?
That would be a very long list.
>> +connect (conn_root,localhost,root,,);
>> +
>> +--disable_warnings
>> +DROP DATABASE IF EXISTS backup_test;
>> +DROP DATABASE IF EXISTS backup_test_alt;
>> +--enable_warnings
>> +
>> +--echo #
>> +--echo # Create users.
>> +--echo #
>> +CREATE USER 'bup_some_priv'@'localhost';
>> +CREATE USER 'joe'@'user';
>
>
> 4) What purpose serves 'joe'? I don't see a real use. I suggest to
> remove to reduce confusion.
Hmmm... I will check.
>> + if (!it)
>> + return BSTREAM_ERROR;
>> +
>> + Image_info::Db *db= (Image_info::Db*)
> info->get_db(it->db->base.pos);
>> + if (!db)
>> + return BSTREAM_ERROR;
>> +
>> + return check_access(thd, priv, db->name().ptr(), 0, 1, 1, 0);
>> +}
>> +
>> +/**
>
>
> 6) Please have two blank lines between function definitions.
Is this a standard? I've never done that. I always use one (but I can
change). ;)
>> +int check_restore_privileges(struct st_bstream_item_info *item,
>> + Restore_info *info,
>> + const char *name_str)
>> +{
>> + using namespace backup;
>> + int result= 0;
>> + THD *thd= ::current_thd;
>> + Image_info::Db *db= NULL;
>> +
>> + Logger &log= info->m_log;
>
>
> 8) Many functions all over the server use 'info', but all have different
> type. For a Restore_info variable, I'd prefer 'rinfo'.
I agree but this is how it was originally coded. I will attempt to be
more specific.
>> + case BSTREAM_IT_DB:
>> + {
>> + result= check_access(thd, CREATE_ACL + DROP_ACL,
>> + name_str, 0, 1, 1, 0);
>> + break;
>> + }
>> + case BSTREAM_IT_TABLE:
>> + case BSTREAM_IT_VIEW:
>> + {
>> + st_bstream_table_info *it= (st_bstream_table_info*)item;
>> + if (!it)
>
>
> 10) At a dozen of places, I have seen a variable 'it' being an iterator.
> here it is a table info pointer. 'ti' would be more logical to use. But
> it is still pretty short. How about 'tinfo'?
Again, habit from existing code. I will attempt to make more clear.
Chuck
