List:Commits« Previous MessageNext Message »
From:Dmitry Shulga Date:December 8 2010 8:24am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450
View as plain text  
Hi Sergey!

On 07.12.2010, at 19:45, Sergey Vojtovich wrote:

> Hi Dmitry,
> 
> a few questions inline...
> 
>> static bool init_line_buffer(LINE_BUFFER *buffer,File file,ulong size,
>> @@ -30,6 +31,15 @@ static char *intern_read_line(LINE_BUFFE
>> LINE_BUFFER *batch_readline_init(ulong max_size,FILE *file)
>> {
>>   LINE_BUFFER *line_buff;
>> +  MY_STAT input_file_stat;
>> +  int input_fd;
>> +
>> +  input_fd= fileno(file);
>> +
>> +  if (my_fstat(input_fd, &input_file_stat, MYF(0)) ||
>> +      (input_file_stat.st_mode & S_IFDIR))
> Why not MY_S_ISDIR(input_file_stat.st_mode)?
> Or at least not (input_file_stat.st_mode & MY_S_IFDIR)?
> What if I run "mysql > abc", or in other words what if
> input_fd is a tty?
Fixed.

> 
>> +    return 0;
>> +
>>   if (!(line_buff=(LINE_BUFFER*)
>>         my_malloc(sizeof(*line_buff),MYF(MY_WME | MY_ZEROFILL))))
>>     return 0;
>> @@ -177,8 +187,15 @@ static size_t fill_buffer(LINE_BUFFER *b
>>   /* Read in new stuff. */
>>   if ((read_count= my_read(buffer->file, (uchar*) buffer->end, read_count,
>> 			   MYF(MY_WME))) == MY_FILE_ERROR)
>> +  {
>> +    /*
>> +      we don't set eof flag for this temporal errors because next
>> +      call to read may be successful.
>> +    */
>> +    if (errno != EAGAIN && errno != EINTR)
> Why not my_errno?
Fixed.

> Why at all to change this code? As far as I can see my_read() handles EINTR.
my_read handles EINTR if THREAD macro is ON. I don't know why did that, so I just added
unconditional handle of EINTR.

> 
>> +      buffer->eof= 1;
>>     return (size_t) -1;
>> -
>> +  }
>>   DBUG_PRINT("fill_buff", ("Got %lu bytes", (ulong) read_count));
>> 
>>   if (!read_count)
>> @@ -222,6 +239,9 @@ char *intern_read_line(LINE_BUFFER *buff
>>       {
>>         if (buffer->eof)
>>           DBUG_RETURN(0);
>> +        // check if syscall read failed with temporal error.
>> +        else if (length == (uint) -1)
>> +          continue;
> Don't you consider this way of returning "temporal error" too complex?
No. Moreover, I think that there is a bug in error handling in current implementation of
buffer filling. In current implementation
if call to my_read failed (when syscall read returned -1), then flag buffer->eof isn't
set to 1. Hence, code inside intern_read_line()
try make next attempt to fill_buffer (and consequently try to read from file descriptor),
although previous read failed and next attempt 
don't make sense.
 
Regards, Dmitry.

Thread
bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450Dmitry Shulga3 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)Bug#57450Sergey Vojtovich7 Dec
    • Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450Dmitry Shulga8 Dec