From: Dmitry Shulga
Date: December 8 2010 8:24am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521) Bug#57450
List-Archive: http://lists.mysql.com/commits/126293
Message-Id: <E6C7F848-B257-452C-BB2C-A796D835C6BF@oracle.com>
MIME-Version: 1.0 (Apple Message framework v1082)
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable

Hi Sergey!

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

> Hi Dmitry,
>=20
> a few questions inline...
>=20
>> 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=3D 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.

>=20
>> +    return 0;
>> +
>>   if (!(line_buff=3D(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=3D my_read(buffer->file, (uchar*) buffer->end, =
read_count,
>> 			   MYF(MY_WME))) =3D=3D MY_FILE_ERROR)
>> +  {
>> +    /*
>> +      we don't set eof flag for this temporal errors because next
>> +      call to read may be successful.
>> +    */
>> +    if (errno !=3D EAGAIN && errno !=3D 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.

>=20
>> +      buffer->eof=3D 1;
>>     return (size_t) -1;
>> -
>> +  }
>>   DBUG_PRINT("fill_buff", ("Got %lu bytes", (ulong) read_count));
>>=20
>>   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 =3D=3D (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=20
don't make sense.
=20
Regards, Dmitry.