From: Sergey Vojtovich
Date: December 16 2010 2:29pm
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Dmitry.Shulga:3521)
 Bug#57450
List-Archive: http://lists.mysql.com/commits/127085
Message-Id: <20101216142948.GA28839@june>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hi Dmirty,

ok to push. There are a few minor concerns inline.
It would be great if Davi could do second review.

On Tue, Dec 14, 2010 at 01:38:20PM +0000, Dmitry Shulga wrote:
> #At file:///Users/shulga/projects/mysql/5.1-bugteam-bug57450/ based on revid:azundris@stripped
> 
>  3521 Dmitry Shulga	2010-12-14
>       Fixed bug#57450	- mysql client enter in an infinite loop
>       if the standard input is a directory.
>       
>       The problem is that mysql monitor try to read from stdin without
>       checking for type of input source.
>       
>       The solution is to check if stdin's file descriptor related to directory
>       and exit from program if this case is true.
>       
>       Additionally, it was fixed a bug in processing of error returned by
>       read syscal.
No test case? But it should be perfectly testable.

>      @ client/readline.cc
>         batch_readline_init() was modified: added checking for file mode
>         and returning from function if input stream is a directory.
>         
>         intern_read_line() was modified: cancel reading from input if
>         fill_buffer() returns -1, i.e. if call to read failed.
s/i.e./e.g./

> 
>     modified:
>       client/readline.cc
> === modified file 'client/readline.cc'
> --- a/client/readline.cc	2009-03-18 08:27:49 +0000
> +++ b/client/readline.cc	2010-12-14 13:38:17 +0000
> @@ -18,6 +18,7 @@
>  #include <my_global.h>
>  #include <my_sys.h>
>  #include <m_string.h>
> +#include <my_dir.h>
>  #include "my_readline.h"
>  
>  static bool init_line_buffer(LINE_BUFFER *buffer,File file,ulong size,
> @@ -30,6 +31,13 @@ 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;
> +
> +  if (my_fstat(fileno(file), &input_file_stat, MYF(0)) ||
> +      MY_S_ISDIR(input_file_stat.st_mode) ||
> +      MY_S_ISBLK(input_file_stat.st_mode))
> +    return 0;
> +
I still didn't manage to get in favour with this hunk, because:
1. AFAICS this is not absolutely necessary, second part of the patch
   solves the problem.
2. It makes readline to depend on another syscall, which is not
   absolutely necessary.
3. I believe it is rather a rare case when people try to load
   data from directory. Was it all worth it?
4. There is no error reported on my_fstat() failure.
5. There is no error reported when input is not applicable.
6. I must admit that I cannot predict all consequencies on all
   supported platforms, but that's ok.

But please don't be blocked, if that's me alone who is not in favour.

Regards,
Sergey

>    if (!(line_buff=(LINE_BUFFER*)
>          my_malloc(sizeof(*line_buff),MYF(MY_WME | MY_ZEROFILL))))
>      return 0;
> @@ -218,11 +226,13 @@ char *intern_read_line(LINE_BUFFER *buff
>          or when the internal buffer has hit the size limit. In the latter case
>          return what we have read so far and signal string truncation.
>        */
> -      if (!(length=fill_buffer(buffer)) || length == (uint) -1)
> +      if (!(length=fill_buffer(buffer)))
>        {
>          if (buffer->eof)
>            DBUG_RETURN(0);
>        }
> +      else if (length == (uint) -1)
> +        DBUG_RETURN(0);
>        else
>          continue;
>        pos--;					/* break line here */
> 


> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=svoj@stripped


-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com