Chuck Bell wrote:
> Rafal,
>
> I can see your points and agree. I think we should have a worklog or bug
> report for error reporting (if we don't already) and include your comments
> in that worklog or bug report.
>
> However, since this bug report is about not overwriting the file, and by
> your own suggestion to implement:
>
> a) ensuring that files are not silently overwritten by BACKUP command
> (which
> is a serious danger);
>
> ... the error reported should state something about not overwriting the file
> -- not some generic error message.
Exactly! The real problem is that existing files are being silently overwritten.
You agree that if we change the kernel so that it doesn't overwrite the files
then it will solve issue a) even if the given error message is too generic?
> I think you should implement the
> my_access() suggestion along with the error message (or something similar)
> that I suggested and take out the checks for location errors. That isn't
> adhoc for two reasons, 1) it is a deliberate addition to the
> execute_backup_command() method that must be a prerequisite for further
> processing, and 2) as you mentioned it is a serious issue that needs to be
> addressed.
It appears to me that this comment is ignoring the main part of my argumentation
in the previous email. There I explained that the addition you propose:
> (kernel.cc)
(...)
> + if (!my_access(lex->backup_dir.str,F_OK))
> + {
> + my_error(ER_BACKUP_CANNOT_OVERWRITE,MYF(0),lex->backup_dir.str);
> + DBUG_RETURN(ER_BACKUP_CANNOT_OVERWRITE);
> + }
assumes that lex->backup_dir contains a path to a file on the local file system.
But the code is designed and written so that it doesn't make this assumption but
treats lex->backup_dir as a general URL describing a location where we want to
store backup image. Using your suggestion would go against this design. For
example, if we use this code and add support for writing to TCP streams, we will
get such behaviour:
BACKUP ... TO 'http://some.server/some/path';
Error: "Attempting to overwrite file 'http://some.server/some/path'"
Which doesn't make much sense. The code is designed in preparation to handle
such situation in the future (by not making too strong assumptions).
I think that this and similar errors should be detected when location is opened
for writing - as this is the moment when we want to access the location and, in
case of files, only in this situation it is an error if the file already exists.
In the general case, for other types of locations, there could be other errors
such as "Can not connect to the server '...'" or "Can not authenticate on the
server '...'" and so on. The place where such things are detected is
open_for_write() function and this is the place where I want to detect "File
already exists" error.
As I explained before, to get the precise description, some error reporting
mechanism must be developed and it is not done yet. I again suggest to leave it
out of the scope of this patch.
As a compromise, I propose to change the ER_BACKUP_WRITE_LOC message to:
ER_BACKUP_WRITE_LOC
eng "Can't write to backup location '%-.64s' (file already exists?)"
This should be appropriate in 99% of the cases and can be easily changed when
proper error reporting is implemented. What do you think?
>
> An error message appropriate to the condition is simply good software
> engineering and one of the best practices thereof. I cannot agree to a patch
> that introduces an error message that is not meaningful to the condition it
> reports. Error messages are for users, not developers.
I disagree that the message is not meaningful. Saying that "Can't write to
backup location '/path/to/the/file'" in the situation when the file exists
states the truth - we can't write to that file. It only doesn't say why, which
doesn't make it meaningless but incomplete.
>
> Lastly, I would strip out the location checking and put it and the location
> error message in the worklog or bug report on backup error handling.
>
I don't understand why I should remove a piece of code which I introduced to
satisfy your review suggestion:
On 11-12-2007 at 21:34 you wrote:
> to figure out what is wrong. I think we should distinguish between at least
> the following conditions:
>
> * ERROR xxxx: File already exists
> * ERROR xxxx: Path or file name error
>
> The first is what this bug report is trying to achieve, but the second can
> be detected as well and is equally as important to detect. It would be much
> better to tell the user which condition is causing the error. Please change
> the patch to accomplish this.
The loc->is_valid() test tries to detect the second error condition.
Rafal
