Hi Chuck,
Status: Approved subject to requested changes.
Requests:
=========
[1] I need an English lesson.
[3] Good. But please add two extra tests for "backupdir" and the two log
files in the other test each.
Suggestions:
============
[2] Not part of your patch. But Rafal just asked me to mask out warning
numbers in tests. If you agree that we should do that, please change it
here as an unrelated fix.
[4] I wonder if we shouldn't have >= here.
[5] I wonder if we shouldn't have >= here.
[7] Good. Just a comment on the variable names in this function.
Ideas:
======
[6] Also it might be good to deduce 2. One for the path delimiter ('/'
or '\') and one for at least one character of a file name. OTOH, if we
create backup files from "backupdir" in local, allocated memory only,
and not in a fixed size array, we may expire FN_REFLEN. Then this idea
is void.
[8] I wonder if it might be a good idea to add an argument here.
Chuck Bell, 25.02.2009 20:58:
...
> 2780 Chuck Bell 2009-02-25
> BUG#42695 : memory leak when setting backupdir
> BUG#42685 : valgrind errors setting backup_progress_log_file
>
> The patch for these bugs both use the new error message and in many
> ways similar so thus the combined patch.
[1] Sorry for my ignorance of English language. I have difficulties to
understand this sentence. Please teach me.
I tried to parse it into its main parts:
The patch ... use the ... message and ... the combined patch.
Er, no, that must be wrong. Here are two parts that seem to make sense
in their self:
... these bugs ... use the new error message ...
... thus the combined patch.
Perhaps one can include 'both' too:
... these bugs both use the new error message ...
... thus the combined patch.
But "bugs" don't "use" messages. So "The patch" has its importance. Even
"The patch for these bugs" sounds clear. But "The patch for these bugs
both" starts to sound odd for my limited understanding. I'd expect
"both" to require a noun in plural. The only plural is "bugs". And yes,
we have two bugs, so "both" and "bugs" must belong together.
And then, the only verb in this sentence is "use". Since it is not
"uses", it must belong to a plural noun. So "bugs" is the only
acceptable partner. But then it would mean that the "bugs both use the
new error message". Well, that could be, though it is a bit sloppy. You
probably mean that "the fixes for the bugs use the error message". Since
you probably want to say that there is just one patch for both bugs, I
cannot think of "fixes", but should think of "fix", but then I would
need to think of "uses" instead of "use". Well, perhaps it is a typo and
you wanted to write "uses". So I would end up with:
The patch for these bugs both uses the new error message ...
Ok. That sounds better. But I still wonder about the placement of
"both". In my understanding, it would belong to the subject of the
sentence, which is the "patch". But it is singular. And we still have
one patch and two bugs. So there is no doubt that "both" belongs to
"bugs". Again, in my limited understanding, I would expect the qualifier
"both" to come before its noun "bugs":
The patch for these both bugs uses the new error message ...
Iiik, sounds ugly, IMHO. In this case I'd assume that "two" needs to be
used instead of "both". However, I can exclude a typo that uses the
wrong word and place it at a wrong position. So I learn that it is
correct English to have "both" where it is. So instead of "for these two
bugs", one can say "for these bugs both". Now, since this seems to be
clear, I'll take the next step. I reduce this part of the sentence to
its core again and include the next part:
The patch ... uses the ... message and in many ways similar ...
The "and" combines two expressions of the same rank. Since the patch
uses the message, it does also use "in many ways similar". Eh? I guess
that "similar" involves two things, so only the "bugs" are available
here. So you probably want to say that the bugs are similar. Or that the
fixes are similar? So one patch - two fixes? But then the first part of
the sentence must have a different meaning from what thought. Anyway, is
it correct to leave out the verb "are" here, from something like "and
are in many ways similar"? But then it would still belong to "The
patch", which is singular. Sigh. Please help.
Finally there is the word "so". Does it belong to "similar", similar to
the placement of "both" in "the bugs both" instead of "both bugs"? So
that you can say "are similar so" instead of "are so similar"? Or does
the "so" belong to the next part: "so thus the combined patch"? Both
"so" and "thus" alone would make this part to express the consequence
from the former part. But both in succession sound odd to me.
So can you please teach me the correct syntactical interpretation of
this sentence? If you were a non-native English speaker, I'd assume the
sentence is plain wrong "in many ways". But since you're native, and
you've been a university teacher, I must assume that I am wrong "in many
ways". So please help me.
...
> --- a/mysql-test/suite/backup/r/backup_backupdir.result 2008-12-24 10:48:24 +0000
> +++ b/mysql-test/suite/backup/r/backup_backupdir.result 2009-02-25 19:58:57 +0000
...
> Attempt to set the backupdir to something invalid.
> SET @@global.backupdir = 'This_is_really_stupid/not/there/at/all';
> Warnings:
> Warning 1733 The path specified for the system variable backupdir cannot be accessed
> or is invalid. ref: This_is_really_stupid/not/there/at/all
> Warning 1733 The path specified for the system variable backupdir cannot be accessed
> or is invalid. ref: This_is_really_stupid/not/there/at/all
[2] Not part of your patch. But Rafal just asked me to mask out warning
numbers in tests. If you agree that we should do that, please change it
here as an unrelated fix.
...
> --- a/mysql-test/suite/backup/t/backup_backupdir.test 2008-12-24 10:48:24 +0000
> +++ b/mysql-test/suite/backup/t/backup_backupdir.test 2009-02-25 19:58:57 +0000
...
> +--echo Attempt to set the backupdir to a really long string.
> +
> +set global max_allowed_packet=1024*100;
> +
> +--error ER_PATH_LENGTH
> +SET @@global.backupdir = repeat('a',99*1024);
[3] Good. But please add two extra tests for "backupdir" and the two log
files in the other test each:
1. A path name that is just one character too long.
2. A path name that has just the maximum allowed length.
You might have chosen the giant length to prevent this test from failing
if some day FN_REFLEN is increased. But I think it is a good thing if
the tests show the consequences of such severe change. The one who does
this must be aware that he might have to fix a lot of things in many places.
...
> --- a/sql/set_var.cc 2009-02-20 16:40:19 +0000
> +++ b/sql/set_var.cc 2009-02-25 19:58:57 +0000
> @@ -2528,6 +2528,16 @@ static int sys_check_log_path(THD *thd,
...
> + if (res->length() > FN_REFLEN)
> + {
> + my_error(ER_PATH_LENGTH, MYF(0), ER(ER_PATH_LENGTH));
> + return 1;
> + }
[4] I wonder if we shouldn't have >= here.
...
> @@ -3002,6 +3012,16 @@ static bool sys_update_backupdir(THD *th
...
> + if (str_length > FN_REFLEN)
> + {
> + my_error(ER_PATH_LENGTH, MYF(0), ER(ER_PATH_LENGTH));
> + return 1;
> + }
[5] I wonder if we shouldn't have >= here.
[6] Also it might be good to deduce 2. One for the path delimiter ('/'
or '\') and one for at least one character of a file name. OTOH, if we
create backup files from "backupdir" in local, allocated memory only,
and not in a fixed size array, we may happily expire FN_REFLEN. Then
this idea is void.
> +
> if (!(res= my_strndup(old_value, str_length, MYF(MY_FAE+MY_WME))))
> {
> result= 1;
> @@ -3010,7 +3030,7 @@ static bool sys_update_backupdir(THD *th
>
> pthread_mutex_lock(&LOCK_global_system_variables);
> logger.lock_exclusive();
> - old_value= sys_var_backupdir.value;
> + my_free(sys_var_backupdir.value, MYF(MY_ALLOW_ZERO_PTR));
> sys_var_backupdir.value= res;
> sys_var_backupdir.value_length= str_length;
> logger.unlock();
[7] Good. Just a comment on the variable names in this function.
"old_value" is now really confusing. Before you change it would become
the old value in the end at least (though it was forgotten to use it
with my_free). But now it does have the new value only. "res" is usually
an integer used to carry the status returned by functions. Here it is
used to carry a copy of the new value. So I'd rather like to have these
variables named as "new_value" and "copied_value" or similar.
>
> === modified file 'sql/share/errmsg.txt'
> --- a/sql/share/errmsg.txt 2009-02-12 17:56:03 +0000
> +++ b/sql/share/errmsg.txt 2009-02-25 19:58:57 +0000
> @@ -6463,4 +6463,5 @@ ER_OPERATION_ABORTED
> eng "Operation aborted"
> ER_OPERATION_ABORTED_CORRUPTED
> eng "Operation aborted - data might be corrupted"
> -
> +ER_PATH_LENGTH
> + eng "The path specified is too long."
[8] I wonder if it might be a good idea to add an argument here:
eng "The path specified for %.64s is too long."
And then insert the variable name when using ER_PATH_LENGTH. This is not
an issue for "SET varname" statements as the name is obvious here. But
if the error message will later be used in other contexts, it might
become handy.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
