Hi Thava,
One comment before the review: Your commit email is missing the
"bundle". This makes it more difficult to apply your patch to a branched
tree. Are you sure that you have the newest version of the mysql plugin?
Currently it is on revision 169.
Status: Approved pending changes.
Required:
1) Build with zlib and fix backup_errors_compression.result.
2) Verify that the error cannot be provoked without error injection.
5) Add a test BACKUP DATABASE * and error injection.
6) Function header formatting.
Options:
3), 4) Select different keywords for error injection.
Details:
Thava Alagu, 06.11.2009 07:36:
...
> 2888 Thava Alagu 2009-11-06
> BUG#47994 : Interruption of BACKUP command reported multiple times.
>
> When backup session is interrupted at certain stages, multiple duplicate
> warning messages are generated.
>
> Added additional checks to catch interruption of the command earlier
> to stop the backup kernel from executing further queries.
> Removed some duplicate pushing of warnings.
...
> === modified file 'mysql-test/suite/backup/r/backup_errors.result'
...
> # Test error handling by injecting errors into BACKUP
> #
> #
> +# Error injection in user privileges checking.
> +#
> +SET SESSION DEBUG='+d,ER_BACKUP_USER_PRIV_CHECK';
> +BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
> +ERROR HY000: Failed to perform access privileges check for user 'root'.
> +SET SESSION DEBUG='-d';
> +#
> # Error injection in si_object.cc:get_database_stub
> #
> SET SESSION DEBUG='+d,siobj_get_db_stub';
> -BACKUP DATABASE db1 TO 'failed1.bak';
> +BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
> ERROR HY000: Failed to add database `db1` to the catalog
> SET SESSION DEBUG='-d';
...
1) You might have a build without zlib, so that backup_errors runs on
your branch, but backup_errors_compression does not. In my environment
the latter runs too. It shows the same result differences as
backup_errors. Can you please try to build with compression and fix that
test result too?
...
> === modified file 'sql/backup/backup_info.cc'
...
> + bool has_access= FALSE;
> + bool got_error= obs::check_user_access(m_thd, name, &has_access);
> +
> + DBUG_EXECUTE_IF("ER_BACKUP_USER_PRIV_CHECK", got_error= TRUE;);
> +
> + if (got_error)
> + {
> + m_log.report_error(ER_BACKUP_USER_PRIV_CHECK, m_thd->security_ctx->user);
> + return NULL;
> + }
2) This is a valid way to inject an error into the code. It is the right
way to test the error branch, if there is no better way to do that.
However, on our last meeting, we agreed that we will always try hard to
find a way to produce an error without error injection first.
In this case it means to find a way to let obs::check_user_access()
return TRUE when called from Backup_info::add_db(). I can imagine that
might not be possible since we have privilege elevation in backup.
Please ask Chuck, if he knows of a way to let obs::check_user_access()
fail in BACKUP. If yes, it would be preferable to do that and not
pollute the code with error injection. If it is not possible, then error
injection is ok to use in the way you did.
3) For the name of the so called "debug keyword", I have my own
considerations. The "error injection points" should be unique. Otherwise
setting the keyword "ER_BACKUP_USER_PRIV_CHECK" could trigger multiple
injection points, and it is difficult to say, what really happened.
As long as there is only one place with
DBUG_EXECUTE_IF("ER_BACKUP_USER_PRIV_CHECK", ...), then everything is
fine. But if someone else is not aware that this does already exist, he
could add a new point with the same keyword.
I use to select the keywords with the function name, in which the error
injection point is located. If a function has more than one injection
point, I add a differentiator. So, in this case I would use
"Backup_info_add_db", or, with differentiator:
"Backup_info_add_db_user_priv".
Since we do not have a standard for this, you are free to use your own
scheme, as long as the keywords remain unique.
> +
> + if (!has_access)
> {
> m_log.report_error(ER_BACKUP_ACCESS_OBJS_INCOMPLETE, name->ptr());
> return NULL;
> @@ -802,7 +813,18 @@ int Backup_info::add_all_dbs()
> int res= 0;
>
> // Check to see if the user can see all of the databases.
> - if (check_user_access(m_thd, 0))
> + bool has_access= FALSE;
> + bool got_error= obs::check_user_access(m_thd, 0, &has_access);
> +
> + DBUG_EXECUTE_IF("ER_BACKUP_USER_PRIV_CHECK", got_error= TRUE;);
4) Ah. Here you have another error injection point with the same keyword
as before. Since add_all_dbs() calls add_db(), where other injection
point is, it may be difficult to hit both. OTOH add_db() is also called
from elsewhere, so there can be another test case that triggers that
one. But in general I prefer to keep the keywords unique.
5) Hm. I made a gcov build and found that this insertion point does not
trigger (but the other one does). This made me aware that there is no
BACKUP DATABASE * test with the insertion point activated.
...
> === modified file 'sql/si_objects.h'
...
> - @param[in] THD The current thread.
> - @param[in] String The database name to check or 0
...
> + @param[in] THD The current thread.
> + @param[in] String The database name to check or 0
> + @param[out] bool Set to true/false as described below:
> + TRUE : User has the required access permission.
> + FALSE : User does not have the required access permission.
> +
> + @return boolean value which indicates if an error has occured.
> + @retval TRUE error has occured.
> + @retval FALSE function completed without error.
6) Now that you touched the function header, can you please make it
conforming with the coding guidelines:
"
...
Parameter specifications in @param section start with lowercase and are
not terminated with a full stop/period.
"
My personal interpretation is that this applies to @retval too (that is,
the trailing dot should be removed).
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
