Hi Chuck,
Here are more detailed comments to your replies.
cbell@stripped wrote:
(cut)
>> See below for more details.
>>> char*); + Backup_info* prepare_for_backup(String *location, +
>>> LEX_STRING orig_loc, +
>>> const char*, bool);
>>> + Restore_info* prepare_for_restore(String *location, +
>>> LEX_STRING orig_loc,
>>> + const char*);
>>
>> Why parameter which holds value of backupdir is called location, not
>> backupdir? Then location could be preserved as a parameter holding,
>> well... the location.
>
> Yeah, ok. I was trying to minimize changes (as per direction) but it can
> be changed -- I will not debate names of parameters -- within reason. ;)
>
Ok, good. I think everyone agrees that 'backupdir' is a better name for
parameter holding the value of backupdir and 'location' is a good name for a
parameter holding the location.
>>> + /*
>>> + Check backupdir for validity (needed since we cannot trust
>>> + that the path has become invalid since set).
>>> + */
>>> + if (backupdir->length() && my_access(backupdir->c_ptr(),
>>> (F_OK|W_OK)))
>>> + {
>>> + context.fatal_error(ER_BACKUP_BACKUPDIR, backupdir->c_ptr());
>>> + DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR,
>>> backupdir->c_ptr()));
>>> + }
>>> +
>>
>> I think it is too strict to always report error here. I would report
>> warning instead and the error will happen when openning stream, but not
>> if user specifies absolute path.
>
> Please check with Peter or Robin. Go with their suggestion/recommendation.
>
Ok, we have it on our agenda to discuss this issue.
>>> +Backup_restore_ctx::prepare_for_backup(String *backupdir, +
>>> LEX_STRING orig_loc, +
>>> const char *query,
>>> bool with_compression)
>>
>> Why renaming location -> orig_loc?
>
> Because it now holds the lex->backup_dir which is the original value
> specified by the user from the command line. If you can think of a
> better name then use it.
>
I think 'location' is a good enough name so no need to change it (think how many
changes in the function body it will save).
>>> + if (prepare(orig_loc))
>>> return NULL;
>>> - backup::String path(location);
>>
>> This variable was here for transforming the string from the
>> representation used in the parser (LEX_STRING) to the representation
>> used in the backup kernel code (String).
>
> Which I now write directly to m_path so I felt it was no longer need
> (and it isn't). If you feel strongly you need to reconvert it then put
> it back in.
Ok, perhaps I missed the m_path value.
>>> +Backup_restore_ctx::prepare_for_restore(String *backupdir,
>>> + LEX_STRING orig_loc, +
>>> const char *query)
>>
>> Why renaming location -> orig_loc?
>
> See above for similar change.
>
>>> /*
>>> Open input stream.
>>> */
>>> - backup::String path(location);
>>
>> This variable was here for transforming the string from the
>> representation used in parser (LEX_STRING) to the representation used
>> in the backup kernel code (String).
>
> See above for similar change.
>
>>> -Stream::Stream(Logger &log, const ::String &name, int flags)
>>> - :m_path(name), m_flags(flags), m_block_size(0), m_log(log)
>>> +Stream::Stream(Logger &log, ::String *backupdir, +
>>> LEX_STRING orig_loc, int flags)
>>> + :m_flags(flags), m_block_size(0), m_log(log)
>>
>> Why "const ::String&" is replaced with "::String*" ?
>> Why renaming name -> orig_loc?
>> Why changing its type to LEX_STRING?
>>
>> I'd prefer to consistently use String for passing string values in the
>> backup code. There was one exception - the parameter of
>> execute_backup_command() was of type LEX_STRING. But this is an
>> exception because the function is called from inside the parser which
>> uses LEX_STRING internaly. Inside backup (kernel) code I wanted to use
>> String class for strings.
>
> Because it does not compile on Windows -- it complains bitterly when
> transforming const to non-const and vice-versa. I also feel strongly we
> should pass pointer to strings and not values. In this case, why string
> by value? Is there something I am not understanding WRT pass by value?
> Isn't passing the pointer faster/easier?
>
Compiler complains most often indicate that the code is poorly written and could
be used as an occasion to improve it (this is what I often do). Here we want to
have const parameters because we don't change them in the function. This is an
important hint for compiler which can be used for better optimization of the code.
I think the only reason for passing pointer to an object (instead of an
reference) is if you want to pass NULL sometimes. In the denotational semantics
language this is a difference between a domain D and a lifted domain D_\bot
which contains all values of D plus one extra NIL value which is different from
all the other. But here we don't want any NULL values - the argument should
always be a concrete string.
When passing a pointer, we should always consider the case that a NULL pointer
is be passed. Thus whenever we dereference the pointer, we should check if it is
not NULL or at least ASSERT that it is the case (which I do in my code). But
here we don't want to deal with NULL pointers so using a reference is simpler.
I'm not passing any parameters by value here. Passing a reference is passing by
reference, not by value. Passing a pointer is also a form of passing by
reference in languages which are so primitive that they don't have explicit "by
refernece" argument passing.
No, passing a pointer is neither faster nor easier than passing a reference.
> I see the "I" pronoun. To me, that means only Rafal can approve such
> changes to the kernel. Please try to think as a team. There is seldom
> one way or even an only way to code. We need to be flexible (perhaps not
> in this case -- I haven't heard your reasoning) with how things are
> done. If they are correct in the sense there is no one correct way we
> shouldn't hold up reviews like we do.
>
There is "I" pronoun becase I wrote the code. So I'm the only person who knows
all the thinking done behind this code. Thus you are lucky to have a reviewer
who can best asses if the changes you propose are in line with the original
design of the code. I think this is advantage, not a disadvantage of my review.
>>> + {
>>> + path_len=backupdir->length() + orig_loc.length + 1;
>>> + m_path.alloc(path_len);
>>> + fn_format(m_path.c_ptr(), orig_loc.str, backupdir->c_ptr(), "",
>>> + MY_UNPACK_FILENAME);
>>> + }
>>> + else
>>> + {
>>> + path_len= orig_loc.length + 1;
>>> + m_path.alloc(path_len);
>>
>> Why do you allocate memory here?
>
> Where does the memory come from? Nothing is allocated yet. If there is a
> better way, please change it.
>
You call m_path.alloc() above for no apparent reason. It will allocate path_len
bytes of memory which then you don't use because m_path.set_ascii() just stores
a pointer to the orig_loc string in the m_path object.
A better way is to not call m_path.alloc().
>>
>>> + m_path.set_ascii(orig_loc.str, orig_loc.length);
>>> + }
>>> + m_path.length(path_len);
>>> +
>>
>> I think m_path.set_ascii(...) sets the length of the string, so this
>> m_patch.length(...) statement is needed only inside if(...) branch,
>> where fn_format(...) is used.
>
> Ok, but when I omitted it the server crashed when m_path was accessed.
> That's because I observered length == 0. Length must be set here.
>
This means I must be wrong about String::set_ascii(...). Although looking at the
code in sql_string.{h,cc} I think I can see that the length of the string is set
there...
>>> + String backupdir;
>>> +
>>> + backupdir.alloc(sys_var_backupdir.value_length);
>>> + backupdir.set_ascii(sys_var_backupdir.value, +
>>> sys_var_backupdir.value_length);
>>> + backupdir.length(sys_var_backupdir.value_length);
>>> +
>>
>> Do you really need to copy the value of the variable? Why not just pass
>> the pointer to the value?
>
> Because the value can change from the time backup is started and the
> time backup needs to access it. By capturing it here we can be sure this
> session of backup/restore has a consistent value when accessed.
>
Ok, I can buy this.
>>> + /*
>>> Note: execute_backup_command() sends a correct response to the
>>> client
>>> (either ok, result set or error message).
>>> - */ - if (execute_backup_command(thd,lex))
>>> + */ + if (execute_backup_command(thd, lex, &backupdir))
>>
>> Since parser uses LEX_STRING for holdings strings, it would be more
>> appropriate to pass backupdir as LEX_STRING. Even if we need to copy
>> the value of backupdir, this can be done inside execute_backup_command.
>
> If you insist...
>
No, not really.
Rafal
