Hi Ingo,
please see my responses below.
Ingo Strüwing wrote:
> Hi Thava,
>
> Status: Undecided.
>
> Required:
> 1) Extended revision comment.
>
OK.
> 3) Explanation, why no existing tests are affected.
>
please see below.
> 5) Use TRUE and FALSE instead of true and false.
>
>
OK.
> Options:
> 2) New test might be unnecessary.
> 4) Change function signature.
>
Please see below.
> Details:
> Thava Alagu, 30.10.2009 01:44:
>
>
>> #At file:///home/thava/mysql/repo/backup/
>>
>> 2853 Thava Alagu 2009-10-30
>> Bug#47994 - Interruption of BACKUP command reported multiple times.
>>
>> Added additional checks to catch interruption of the command earlier
>> to stop the backup kernel from executing further queries.
>> Removed some duplicate pushing of warningss.
[ cut ] ----
>
> 2) It is good to add tests for a bug fix. Unless they duplicate already
> present tests. If I understand correctly, the bug was reported because
> existing tests showed this behavior. So it seems that it could be
> sufficient to run it against existing tests. Or do your tests add
> something that is not covered by other tests?
>
>
There is an existing test which tests for the effect of interrupting
backup at different synchronization points. This fix adds and enables
the new synchronization point (which was never enabled earlier in the test)
in the existing test effectively testing the fix.
> 3) OTOH I wonder, why only new tests are added, and no warnings are
> removed from other tests. I am sure that I added some duplicate
> "interrupted" warnings to some test cases. I think that this patch
> should remove them. So I doubt that the problem is really fixed.
>
I did check for such duplicates in other tests and didn't
find any. please let me know if I have missed anything.
> ...
>
>> === modified file 'sql/si_objects.cc'
>>
> ...
>
>> ///////////////////////////////////////////////////////////////////////////
>> -bool check_user_access(THD *thd, const String *db_name)
>> +bool check_user_access(THD *thd, const String *db_name, bool *rc)
>>
>
>
> 4) I would like to urge you, to exchange error and result. IMHO, it is
> more MySQLish and more to my liking that a function, which can fail,
> should report success/failure through its return value. Additional
> information (here: access_granted) should either be compatible with the
> success/error status (and thus combined), or passed by a function
> argument. Here we have two booleans. It won't be natural to combine
> them, though this is often done as +/0/- return. So I suggest to let the
> function return success/failure and have a 'access_granted' parameter.
> Fortunately, the name of the function does not trick one to think it
> would return the access status.
>
> (And, btw, the parameter name 'rc' did not transport the idea if TRUE
> means success or error.)
>
>
I will make this change since I think renaming of variables
is more readable. If it were a get_xxx() function, I would suggest
to pass got_error as a variable instead of rc. It is lot less confusing
that way to use TRUE to flag error.
-thava
