List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:October 6 2010 3:48am
Subject:Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)
Bug#29071
View as plain text  
Hi Kristofer,

it is seven days since I sent my first comments to this patch.
During this time I failed in many regards:
I failed to understand why the original code is bad.
I failed to understand why the proposed code is good.
I failed to defeat my fears that the proposed code is risky.
I failed to be understandable enough to explain my point.
I failed to force myself to approve what I don't understand.
I failed to afford myself to further block rapid progress.

I'm afraid I'm not qualified to be reviewer for this patch. I'm
resigning from this job. I'd suggest to work towards with Kostja,
Wlad or Ingo - I'm sure they have great qualification in this area.

Regards,
Sergey

On Tue, Oct 05, 2010 at 12:38:44PM +0000, Kristofer Pettersson wrote:
> #At file:///home/thek/bzr/mysql-trunk/ based on
> revid:dlenev@stripped
> 
>  3197 Kristofer Pettersson	2010-10-05
>       Bug#29071 invalid Open_files or Slave_open_temp_tables value in show global
> status
>       
>       If an error occurred during registration of
>       a file handle, the Open_files counter would
>       show very large numbers.
>       
>       Only decrease my_file_opened or my_stream_opened if the file
>       descriptor was maked as anything but UNOPEN
>      @ include/my_global.h
>         * In order to be able to map all file descriptors to the minimal requirement
> of concurrently open files we need to increase MY_NFILE to 256 which also matches the
> official recommendation better.
>      @ mysql-test/t/myisampack.test
>         * Add test file to verify that previously working scripts are still working
> within acceptable limits.
>      @ mysys/my_file.c
>         * Make sure that the my_file_limit is updated even if the requested number of
> file descriptors fit into the default static memory allocation.
>         * If an allocation fail, make sure the my_file_info pointer is reverted to
> the default structure.
>      @ mysys/my_fopen.c
>         * The variable "my_stream_opened" needs to be strongly associated
>           with the my_file_info::type attribute.
>         * Move the THR_LOCK_open mutex so that we don't hold it while doing system
> calls.
>      @ mysys/my_open.c
>         * The variable "my_file_opened" needs to be strongly associated
>           with the my_file_info::type attribute.
>         * Move the THR_LOCK_open mutex so that we don't hold it while doing system
> calls.
>         * Previously the file descriptor didn't map strictly to the my_file_info
> array and when very many files were used, the my_file_info array weren't updated.
>      @ unittest/gunit/CMakeLists.txt
>         * Added two test cases my_open-t.cc, my_fopen-t.cc
> 
>     added:
>       unittest/gunit/my_fopen-t.cc
>       unittest/gunit/my_open-t.cc
>     modified:
>       include/my_global.h
>       mysql-test/t/myisampack.test
>       mysys/my_file.c
>       mysys/my_fopen.c
>       mysys/my_open.c
>       unittest/gunit/CMakeLists.txt
> === modified file 'include/my_global.h'
> --- a/include/my_global.h	2010-07-23 20:59:42 +0000
> +++ b/include/my_global.h	2010-10-05 12:38:33 +0000
> @@ -642,7 +642,7 @@ typedef SOCKET_SIZE_TYPE size_socket;
>  #ifdef _WIN32
>  #define MY_NFILE (16384 + MY_FILE_MIN)
>  #else
> -#define MY_NFILE 64
> +#define MY_NFILE 256
>  #endif
>  
>  #ifndef OS_FILE_LIMIT
> 
> === modified file 'mysql-test/t/myisampack.test'
> --- a/mysql-test/t/myisampack.test	2009-11-26 12:47:55 +0000
> +++ b/mysql-test/t/myisampack.test	2010-10-05 12:38:33 +0000
> @@ -221,3 +221,61 @@ DROP TABLE t1,t2,t3;
>  DROP TABLE mysql_db1.t1;
>  DROP DATABASE mysql_db1;
>  
> +#
> +# Bug#29071 invalid Open_files or Slave_open_temp_tables value in show global
> +#           status
> +#
> +let $MYSQLD_DATADIR= `select @@datadir`;
> +disable_query_log;
> +let $ntables= 64;
> +let $i= $ntables;
> +let $myisampack_arg=-j $MYSQLD_DATADIR/test/t0;
> +while ($i)
> +{
> +  eval CREATE TABLE t$i(a INT);
> +  let $myisampack_arg=$myisampack_arg $MYSQLD_DATADIR/test/t$i;
> +  dec $i;
> +}
> +FLUSH TABLES;
> +enable_query_log;
> +
> +# myisampack
> +--exec $MYISAMPACK $myisampack_arg
> +
> +disable_query_log;
> +let $i= $ntables;
> +while ($i)
> +{
> +  eval DROP TABLE t$i;
> +  dec $i;
> +}
> +disable_warnings;
> +DROP TABLE IF EXISTS t0;
> +enable_warnings;
> +enable_query_log;
> +
> +let $i= $ntables;
> +while ($i)
> +{
> +  connect(con$i,localhost,root,,);
> +  dec $i;
> +}
> +
> +disable_query_log;
> +CREATE TABLE t1(a INT);
> +# mysqltest
> +cat_file $MYSQLD_DATADIR/test/t1.MYD;
> +
> +# libmysqlclient
> +eval LOAD DATA LOCAL INFILE '$MYSQLD_DATADIR/test/t1.MYD' INTO TABLE t1;
> +DROP TABLE t1;
> +enable_query_log;
> +
> +let $i= $ntables;
> +while ($i)
> +{
> +  disconnect con$i;
> +  dec $i;
> +}
> +
> +
> 
> === modified file 'mysys/my_file.c'
> --- a/mysys/my_file.c	2010-07-08 21:20:08 +0000
> +++ b/mysys/my_file.c	2010-10-05 12:38:33 +0000
> @@ -83,12 +83,10 @@ static uint set_max_open_files(uint max_
>  /*
>    Change number of open files
>  
> -  SYNOPSIS:
> -    my_set_max_open_files()
> -    files		Number of requested files
> +  @param files Number of requested files
>  
> -  RETURN
> -    number of files available for open
> +  
> +  @return Number of files available for open
>  */
>  
>  uint my_set_max_open_files(uint files)
> @@ -100,11 +98,26 @@ uint my_set_max_open_files(uint files)
>    files+= MY_FILE_MIN;
>    files= set_max_open_files(min(files, OS_FILE_LIMIT));
>    if (files <= MY_NFILE)
> +  {
> +    /*
> +      The number of requested file descriptors will fit into the default
> +      static allocation.
> +    */
> +    my_file_info= my_file_info_default;
> +    my_file_limit= files;
>      DBUG_RETURN(files);
> -
> +  }
>    if (!(tmp= (struct st_my_file_info*) my_malloc(sizeof(*tmp) * files,
>  						 MYF(MY_WME))))
> +  {
> +    /*
> +      if allocation failed then revert to the default number of file
> +      descriptors.
> +    */
> +    my_file_info= my_file_info_default;
> +    my_file_limit= MY_NFILE;
>      DBUG_RETURN(MY_NFILE);
> +  }
>  
>    /* Copy any initialized files */
>    memcpy((char*) tmp, (char*) my_file_info,
> @@ -126,7 +139,7 @@ void my_free_open_file_info()
>    {
>      /* Copy data back for my_print_open_files */
>      memcpy((char*) my_file_info_default, my_file_info,
> -           sizeof(*my_file_info_default)* MY_NFILE);
> +           sizeof(my_file_info_default));
>      my_free(my_file_info);
>      my_file_info= my_file_info_default;
>      my_file_limit= MY_NFILE;
> 
> === modified file 'mysys/my_fopen.c'
> --- a/mysys/my_fopen.c	2010-07-08 21:20:08 +0000
> +++ b/mysys/my_fopen.c	2010-10-05 12:38:33 +0000
> @@ -20,18 +20,33 @@
>  
>  static void make_ftype(char * to,int flag);
>  
> -/*
> +/**
>    Open a file as stream
>  
> -  SYNOPSIS
> -    my_fopen()
> -    FileName	Path-name of file
> -    Flags	Read | write | append | trunc (like for open())
> -    MyFlags	Flags for handling errors
> -
> -  RETURN
> -    0	Error
> -    #	File handler
> +  @param FileName Path-name of file
> +  @param Flags like for open()
> +  @param MyFlags Flags for handling errors
> +
> + The following global variables are read:
> +    struct st_my_file_info *my_file_info
> +    my_file_limit
> +  
> +  The following global variables are written:
> +    struct st_my_file_info *my_file_info
> +    my_stream_opened
> +    my_total_opened  
> +    my_errno
> + 
> +  This function attempts to acquire the THR_LOCK_open mutex.
> +  
> +  This function can throw the following SQL errors:
> +    EE_FILENOTFOUND
> +    EE_CANTCREATEFILE
> +    EE_OUT_OF_FILERESOURCES
> + 
> +  @return
> +    @retval 0 Error
> +    @retval # File handler
>  */
>  
>  FILE *my_fopen(const char *filename, int flags, myf MyFlags)
> @@ -51,21 +66,23 @@ FILE *my_fopen(const char *filename, int
>  #endif
>    if (fd != 0)
>    {
> -    /*
> -      The test works if MY_NFILE < 128. The problem is that fileno() is char
> -      on some OS (SUNOS). Actually the filename save isn't that important
> -      so we can ignore if this doesn't work.
> -    */
> -
>      int filedesc= my_fileno(fd);
> -    if ((uint)filedesc >= my_file_limit)
> +    mysql_mutex_lock(&THR_LOCK_open); 
> +    if ((uint)filedesc >= (uint)my_file_limit)
>      {
> -      thread_safe_increment(my_stream_opened,&THR_LOCK_open);
> -      DBUG_RETURN(fd);				/* safeguard */
> +      /*
> +        The number of concurrently opened files exceeds the limits or
> +        the file descriptor can't be mapped to the my_file_info array.
> +      */
> +      my_errno= EMFILE;
> +      fclose(fd);
> +#ifdef _WIN32
> +      my_file_info[filedesc].fhandle= 0;   
> +#endif
> +      mysql_mutex_unlock(&THR_LOCK_open);
> +      goto err;
>      }
> -    mysql_mutex_lock(&THR_LOCK_open);
> -    if ((my_file_info[filedesc].name= (char*)
> -	 my_strdup(filename,MyFlags)))
> +    if ((my_file_info[filedesc].name= (char*) my_strdup(filename,MyFlags)))
>      {
>        my_stream_opened++;
>        my_file_total_opened++;
> @@ -80,6 +97,8 @@ FILE *my_fopen(const char *filename, int
>    }
>    else
>      my_errno=errno;
> +    
> +err:
>    DBUG_PRINT("error",("Got error %d on open",my_errno));
>    if (MyFlags & (MY_FFNF | MY_FAE | MY_WME))
>      my_error((flags & O_RDONLY) || (flags == O_RDONLY ) ? EE_FILENOTFOUND :
> @@ -89,22 +108,35 @@ FILE *my_fopen(const char *filename, int
>  } /* my_fopen */
>  
>  
> -	/* Close a stream */
> +/**
> +  Close a stream
> +  @param fd A pointer to a stream file descriptor
> +  @param MyFlags Flags regulating how errors are treated.
> +
> +  @note This function must be called with a valid file descriptor or it will
> +    crash. A valid fd is defined on the intervall ]-1, my_file_limit[
> +  
> +  @see my_win_fclose
> + 
> +  The following global variables are read:
> +    struct st_my_file_info *my_file_info
> +
> +  The following global variables are written:
> +   struct st_my_file_info *my_file_info     
> +   my_stream_opened
> +   
> +  This function can throw the following SQL errors:
> +    EE_BADCLOSE
> +*/
>  
> -/* Close a stream */
>  int my_fclose(FILE *fd, myf MyFlags)
>  {
>    int err,file;
>    DBUG_ENTER("my_fclose");
>    DBUG_PRINT("my",("stream: 0x%lx  MyFlags: %d", (long) fd, MyFlags));
>  
> -  mysql_mutex_lock(&THR_LOCK_open);
>    file= my_fileno(fd);
> -#ifndef _WIN32
>    err= fclose(fd);
> -#else
> -  err= my_win_fclose(fd);
> -#endif
>    if(err < 0)
>    {
>      my_errno=errno;
> @@ -112,12 +144,21 @@ int my_fclose(FILE *fd, myf MyFlags)
>        my_error(EE_BADCLOSE, MYF(ME_BELL+ME_WAITTANG),
>  	       my_filename(file),errno);
>    }
> -  else
> -    my_stream_opened--;
> -  if ((uint) file < my_file_limit && my_file_info[file].type != UNOPEN)
> +  
> +  mysql_mutex_lock(&THR_LOCK_open);
> +#ifdef _WIN32
> +  /*
> +    The windows compatibility frame work attaches a handle which needs to be
> +    set to 0 when the file is closed.
> +    @see my_win_fclose()
> +  */
> +  my_file_info[file].fhandle= 0;
> +#endif
> +  if (my_file_info[file].type != UNOPEN)
>    {
>      my_file_info[file].type = UNOPEN;
>      my_free(my_file_info[file].name);
> +    my_stream_opened--;
>    }
>    mysql_mutex_unlock(&THR_LOCK_open);
>    DBUG_RETURN(err);
> 
> === modified file 'mysys/my_open.c'
> --- a/mysys/my_open.c	2010-07-08 21:20:08 +0000
> +++ b/mysys/my_open.c	2010-10-05 12:38:33 +0000
> @@ -19,17 +19,37 @@
>  #include <errno.h>
>  
>  
> -/*
> +/**
>    Open a file
>  
> -  SYNOPSIS
> -    my_open()
> -      FileName	Fully qualified file name
> -      Flags	Read | write 
> -      MyFlags	Special flags
> -
> -  RETURN VALUE
> -    File descriptor
> +  @param FileName Fully qualified file name
> +  @param Flags Same as for unix system call open(2)
> +  @param MyFlags Special flags
> +
> +  @note This function encapsluates my_win_open and open(3) calls.
> +  
> +  The following global variables are read:
> +    struct st_my_file_info *my_file_info
> +    my_file_limit
> +  
> +  The following global variables are written:
> +    struct st_my_file_info *my_file_info
> +    my_file_opened
> +    my_total_opened
> +    my_errno
> +    PSI_mutex_key key_my_file_info_mutex (initialized)
> + 
> +  This function attempts to acquire the THR_LOCK_open mutex.
> +  
> +  This function can throw the following SQL errors:
> +    EE_FILENOTFOUND
> +    EE_OUT_OF_FILERESOURCES
> + 
> +  @see my_win_open
> + 
> +  @return
> +    @retval -1 An error occurred; my_errno has the error code.
> +    @retval #  On success a file descriptor is returned.
>  */
>  
>  File my_open(const char *FileName, int Flags, myf MyFlags)
> @@ -55,13 +75,17 @@ File my_open(const char *FileName, int F
>  
>  
>  /*
> -  Close a file
> -
> -  SYNOPSIS
> -    my_close()
> -      fd	File sescriptor
> -      myf	Special Flags
> -
> +  Close a file.
> +  
> +  @param fd File sescriptor
> +  @param myf Flags regulating how errors are treated.
> +  
> +  The following global variables are read:
> +    struct st_my_file_info *my_file_info
> +    my_file_limit
> +    
> +  This function can throw the following SQL errors:
> +    EE_BADCLOSE
>  */
>  
>  int my_close(File fd, myf MyFlags)
> @@ -70,13 +94,25 @@ int my_close(File fd, myf MyFlags)
>    DBUG_ENTER("my_close");
>    DBUG_PRINT("my",("fd: %d  MyFlags: %d",fd, MyFlags));
>  
> -  mysql_mutex_lock(&THR_LOCK_open);
> +  if (fd < 0 || fd >= my_file_limit)
> +  {
> +    /* Function called with wrong parameters */
> +    DBUG_RETURN(EBADF);  
> +  }
> +
>  #ifndef _WIN32
>    do
>    {
>      err= close(fd);
>    } while (err == -1 && errno == EINTR);
> -#else
> +#endif
> +
> +  mysql_mutex_lock(&THR_LOCK_open);
> +#ifdef _WIN32
> +  /*
> +    my_win_close() calls invalidate_fd() which in turn modifies the my_file_info
> +    structure which must be protected by THR_LOCK_open.
> +  */
>    err= my_win_close(fd);
>  #endif
>    if (err)
> @@ -86,15 +122,15 @@ int my_close(File fd, myf MyFlags)
>      if (MyFlags & (MY_FAE | MY_WME))
>        my_error(EE_BADCLOSE, MYF(ME_BELL+ME_WAITTANG),my_filename(fd),errno);
>    }
> -  if ((uint) fd < my_file_limit && my_file_info[fd].type != UNOPEN)
> +  if (my_file_info[fd].type != UNOPEN)
>    {
>      my_free(my_file_info[fd].name);
>  #if defined(THREAD) && !defined(HAVE_PREAD) && !defined(_WIN32)
>      mysql_mutex_destroy(&my_file_info[fd].mutex);
>  #endif
>      my_file_info[fd].type = UNOPEN;
> +    my_file_opened--;
>    }
> -  my_file_opened--;
>    mysql_mutex_unlock(&THR_LOCK_open);
>    DBUG_RETURN(err);
>  } /* my_close */
> @@ -103,17 +139,17 @@ int my_close(File fd, myf MyFlags)
>  /*
>    Register file in my_file_info[]
>     
> -  SYNOPSIS
> -    my_register_filename()
> -    fd			   File number opened, -1 if error on open
> -    FileName		   File name
> -    type_file_type	   How file was created
> -    error_message_number   Error message number if caller got error (fd == -1)
> -    MyFlags		   Flags for my_close()
> -
> -  RETURN
> -    -1   error
> -     #   Filenumber
> +  @param fd File number opened, -1 if error on open
> +  @param FileName File name
> +  @param type_file_type How file was created
> +  @param error_message_numbe Error message number if caller got error (fd == -1)
> +  @param MyFlags Flags for my_close()
> +
> +  @see my_open
> +  
> +  @return
> +    @retval -1 An error occurred
> +    @retval # On success the file descriptor is returned.
>  
>  */
>  
> @@ -121,41 +157,56 @@ File my_register_filename(File fd, const
>  			  type_of_file, uint error_message_number, myf MyFlags)
>  {
>    DBUG_ENTER("my_register_filename");
> -  if ((int) fd >= MY_FILE_MIN)
> +  if (fd < 0)
>    {
> -    if ((uint) fd >= my_file_limit)
> +    /*
> +      The file descriptor indicates that the FileName couldn't be opened
> +      properly; - Report the error.
> +    */
> +    my_errno= errno;
> +    goto err;
> +  }
> + 
> +  mysql_mutex_lock(&THR_LOCK_open); 
> +  if ((uint)fd >= (uint)my_file_limit)
> +  {
> +    /*
> +      The number of concurrently opened files exceeds the limits or
> +      the file descriptor can't be mapped to the my_file_info array.
> +    */
> +    my_errno= EMFILE;
> +#ifndef _WIN32
> +    int ret;
> +    do
>      {
> -#if defined(THREAD) && !defined(HAVE_PREAD) 
> -      my_errno= EMFILE;
> +      ret= close(fd);
> +    } while (ret == -1 && errno == EINTR);
>  #else
> -      thread_safe_increment(my_file_opened,&THR_LOCK_open);
> -      DBUG_RETURN(fd);				/* safeguard */
> +    my_win_close(fd);
>  #endif
> -    }
> -    else
> -    {
> -      mysql_mutex_lock(&THR_LOCK_open);
> -      if ((my_file_info[fd].name = (char*) my_strdup(FileName,MyFlags)))
> -      {
> -        my_file_opened++;
> -        my_file_total_opened++;
> -        my_file_info[fd].type = type_of_file;
> -#if defined(THREAD) && !defined(HAVE_PREAD) && !defined(_WIN32)
> -        mysql_mutex_init(key_my_file_info_mutex, &my_file_info[fd].mutex,
> -                         MY_MUTEX_INIT_FAST);
> -#endif
> -        mysql_mutex_unlock(&THR_LOCK_open);
> -        DBUG_PRINT("exit",("fd: %d",fd));
> -        DBUG_RETURN(fd);
> -      }
> -      mysql_mutex_unlock(&THR_LOCK_open);
> -      my_errno= ENOMEM;
> -    }
> -    (void) my_close(fd, MyFlags);
> +    mysql_mutex_unlock(&THR_LOCK_open);
> +    goto err;
>    }
> -  else
> -    my_errno= errno;
>  
> +  if ((my_file_info[fd].name = (char*) my_strdup(FileName,MyFlags)))
> +  {
> +    my_file_opened++;
> +    my_file_total_opened++;
> +    my_file_info[fd].type = type_of_file;
> +#if defined(THREAD) && !defined(_WIN32) && !defined(HAVE_PREAD)
> +    mysql_mutex_init(key_my_file_info_mutex, &my_file_info[fd].mutex,
> +                     MY_MUTEX_INIT_FAST);
> +#endif
> +    mysql_mutex_unlock(&THR_LOCK_open);
> +    DBUG_PRINT("exit",("fd: %d",fd));
> +    DBUG_RETURN(fd);
> +  }
> +#if defined(THREAD)
> +  mysql_mutex_unlock(&THR_LOCK_open);
> +#endif
> +  my_errno= ENOMEM;
> +
> +err:
>    DBUG_PRINT("error",("Got error %d on open", my_errno));
>    if (MyFlags & (MY_FFNF | MY_FAE | MY_WME))
>    {
> @@ -190,3 +241,4 @@ void my_print_open_files(void)
>  }
>  
>  #endif
> +
> 
> === modified file 'unittest/gunit/CMakeLists.txt'
> --- a/unittest/gunit/CMakeLists.txt	2010-07-30 08:34:23 +0000
> +++ b/unittest/gunit/CMakeLists.txt	2010-10-05 12:38:33 +0000
> @@ -207,7 +207,7 @@ IF (CMAKE_CXX_COMPILER_ID STREQUAL "SunP
>  ENDIF()
>  
>  # Add tests (link them with sql library) 
> -SET(TESTS sql_list mdl mdl_mytap my_regex thread_utils)
> +SET(TESTS sql_list mdl mdl_mytap my_regex thread_utils my_open my_fopen string)
>  FOREACH(test ${TESTS})
>    ADD_EXECUTABLE(${test}-t ${test}-t.cc)
>    TARGET_LINK_LIBRARIES(${test}-t gunit sqlgunitlib strings dbug regex)
> 
> === added file 'unittest/gunit/my_fopen-t.cc'
> --- a/unittest/gunit/my_fopen-t.cc	1970-01-01 00:00:00 +0000
> +++ b/unittest/gunit/my_fopen-t.cc	2010-10-05 12:38:33 +0000
> @@ -0,0 +1,174 @@
> +/* Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software Foundation,
> +   51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA */
> +
> +#include "my_config.h"
> +#include <gtest/gtest.h>
> +#include "mysqld_error.h"
> +#include "my_sys.h"
> +
> +#define MAX_NUMBER_OF_FILE_DESCRIPTORS (OS_FILE_LIMIT+MY_NFILE+1024)
> +#define DUMMY_FILE_NAME "successful_open_rdonly.bin"
> +
> +struct st_my_file_info my_file_info_default[MY_NFILE];
> +class My_open_test : public ::testing::Test
> +{
> +public:
> +  My_open_test()
> +  {
> +  }
> +  
> +  virtual void SetUp()
> +  {
> +    /* set all global variables for each TEST_F. */
> +    my_thread_basic_global_reinit();
> +    my_stream_opened= 0;
> +    my_file_total_opened= 0;
> +    my_file_info= my_file_info_default;
> +    my_set_max_open_files(MAX_NUMBER_OF_FILE_DESCRIPTORS);
> +    my_errno= 0;
> +  }
> +  
> +  /**
> +    Set up test case for all tests
> +  */
> +  static void SetUpTestCase()
> +  {
> +    my_thread_basic_global_init();
> +    FILE *fp;
> +    fp= my_fopen(DUMMY_FILE_NAME, O_WRONLY | O_CREAT | O_BINARY, MYF(0));
> +    if (fp == 0)
> +    {
> +      ADD_FAILURE() << "Failed to SetUpTestCase().";
> +      exit(EXIT_FAILURE);
> +    }
> +    my_fclose(fp, MYF(0));
> +  }
> +  
> +  static void TearDownTestCase()
> +  {
> +    /*
> +      The std lib routine unlink(3) is considered trusted.
> +    */
> +    if (unlink(DUMMY_FILE_NAME) > 0)
> +    {
> +      ADD_FAILURE() << "Failed to TearDownTestCase().";
> +      exit(EXIT_FAILURE);
> +    }
> +  }
> +
> +private:
> +
> +};
> +
> +TEST_F(My_open_test, EnvironmentCheck)
> +{
> +}
> +
> +TEST_F(My_open_test, FailToOpenUnknownFile)
> +{
> +  FILE *fp;
> +  fp= my_fopen("/really/unknown/does-not-exist.bin", O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_EQ((FILE *)0, fp);
> +  EXPECT_EQ(0, my_stream_opened);
> +  EXPECT_EQ(0, my_file_total_opened);
> +}
> +
> +
> +TEST_F(My_open_test, SuccessToOpenKnownBinaryFile)
> +{
> +  EXPECT_EQ(0, my_stream_opened);
> +  FILE *fp;
> +  fp= my_fopen(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +  /*
> +    The file should exist in the test dir and the call to my_open should not
> +    fail under normal circumstances.
> +  */
> +  EXPECT_NE((FILE *)0, fp);
> +  
> +  /*
> +    The number of currently opened files should have increased
> +  */
> +  EXPECT_EQ(1, my_stream_opened);
> +  /*
> +    The number of total files should have increased
> +  */
> +  EXPECT_EQ(1, my_file_total_opened);
> +  
> +  
> +  /*
> +    When the file is closed the number of currently opened files should have
> +    decreased to 0.
> +  */
> +  my_fclose(fp, MYF(0));
> +  EXPECT_EQ(0, my_stream_opened);  
> +
> +  /*
> +    But the number of files which has been opened should of course stay the
> +    same.
> +  */
> +  EXPECT_EQ(1, my_file_total_opened);
> +  
> +  /*
> +    Lets try it again to see if the number of files ever opened in the system
> +    increases to 2
> +  */
> +  fp= my_fopen(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));  
> +  EXPECT_NE((FILE *)0, fp);
> +  EXPECT_EQ(1, my_stream_opened);
> +  EXPECT_EQ(2, my_file_total_opened);
> +  my_fclose(fp, MYF(0));
> +  EXPECT_EQ(0, my_stream_opened);  
> +  EXPECT_EQ(2, my_file_total_opened);
> +  
> +}
> +
> +TEST_F(My_open_test, TooManyOpenFiles)
> +{
> +  FILE *fp[MAX_NUMBER_OF_FILE_DESCRIPTORS+1];
> +  /*
> +    Attempt to open MAX_NUMBER_OF_FILE_DESCRIPTORS number of files.
> +    NOTE: Couting begins at 0 hence we exclude i==MAX_NUMBER_OF_FILE_DESCRIPTORS
> +  */
> +  int opened_files= 0;
> +  for(unsigned i= 0; i<MAX_NUMBER_OF_FILE_DESCRIPTORS; ++i)
> +  {
> +    fp[i]= my_fopen(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +    if (fp[i] == 0)
> +      break;
> +    else
> +      ++opened_files;
> +  }
> +  /*
> +    We now have depleted the number of available file descriptors.
> +    Next attempt to open a file will fail.
> +  */
> +  fp[opened_files]= my_fopen(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_EQ((FILE *)0, fp[opened_files]);
> +
> +  /*
> +    Apply my_fclose() on all file descriptors to make sure all files are closed
> +  */
> +  for(int i= opened_files-1; i>=0; --i)
> +  {
> +    if( fp[i] != 0)
> +      my_fclose(fp[i],MYF(0));
> +  }
> +  /*
> +    No more files should be opened.
> +  */
> +  EXPECT_EQ(0, my_stream_opened);
> +  EXPECT_EQ(opened_files, my_file_total_opened);
> +}
> +
> 
> === added file 'unittest/gunit/my_open-t.cc'
> --- a/unittest/gunit/my_open-t.cc	1970-01-01 00:00:00 +0000
> +++ b/unittest/gunit/my_open-t.cc	2010-10-05 12:38:33 +0000
> @@ -0,0 +1,183 @@
> +/* Copyright (c) 2000, 2010, Oracle and/or its affiliates. All rights reserved.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software Foundation,
> +   51 Franklin Street, Suite 500, Boston, MA 02110-1335 USA */
> +
> +#include "my_config.h"
> +#include <gtest/gtest.h>
> +#include "mysqld_error.h"
> +#include "my_sys.h"
> +
> +#define MAX_NUMBER_OF_FILE_DESCRIPTORS (OS_FILE_LIMIT+MY_NFILE+1024)
> +#define DUMMY_FILE_NAME "successful_open_rdonly.bin"
> +
> +struct st_my_file_info my_file_info_default[MY_NFILE];
> +class My_open_test : public ::testing::Test
> +{
> +public:
> +  My_open_test()
> +  {
> +  }
> +  
> +  virtual void SetUp()
> +  {
> +    /* set all global variables for each TEST_F */
> +    my_thread_basic_global_reinit();
> +    my_file_opened= 0;
> +    my_file_total_opened= 0;
> +    my_file_info= my_file_info_default;
> +    my_set_max_open_files(MAX_NUMBER_OF_FILE_DESCRIPTORS);
> +    my_errno= 0;
> +  }
> +  
> +  static void SetUpTestCase()
> +  {
> +    my_thread_basic_global_init();
> +    File fd;
> +    fd= my_open(DUMMY_FILE_NAME, O_WRONLY | O_CREAT | O_BINARY, MYF(0));
> +    if (fd == -1)
> +    {
> +      ADD_FAILURE() << "Failed to SetUpTestCase().";
> +      exit(EXIT_FAILURE);
> +    }
> +    my_close(fd, MYF(0));
> +  }
> +  
> +  static void TearDownTestCase()
> +  {
> +    /*
> +      The std lib routine unlink(3) is considered trusted.
> +    */
> +    if (unlink(DUMMY_FILE_NAME) > 0)
> +    {
> +      ADD_FAILURE() << "Failed to TearDownTestCase().";
> +      exit(EXIT_FAILURE);
> +    }
> +  }
> +
> +private:
> +
> +};
> +
> +TEST_F(My_open_test, FailToOpenUnknownFile)
> +{
> +  File fd;
> +  fd= my_open("/really/unknown/does-not-exist.bin", O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_EQ(-1, fd);
> +  EXPECT_EQ(0, my_file_opened);
> +  EXPECT_EQ(0, my_file_total_opened);
> +}
> +
> +
> +TEST_F(My_open_test, SuccessToOpenKnownBinaryFile)
> +{
> +  File fd;
> +  fd= my_open(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +  /*
> +    The file should exist in the test dir and the call to my_open should not
> +    fail under normal circumstances.
> +  */
> +  EXPECT_NE(-1, fd);
> +  
> +  /*
> +    The number of currently opened files should have increased
> +  */
> +  EXPECT_EQ(1, my_file_opened);
> +  /*
> +    The number of total files should have increased
> +  */
> +  EXPECT_EQ(1, my_file_total_opened);
> +  
> +  my_close(fd, MYF(0));
> +  
> +  /*
> +    When the file is closed the number of currently opened files should have
> +    decreased to 0.
> +  */
> +  EXPECT_EQ(0, my_file_opened);  
> +  /*
> +    But the number of files which has been opened should of course stay the
> +    same.
> +  */
> +  EXPECT_EQ(1, my_file_total_opened);
> +  
> +  /*
> +    Lets try it again to see if the number of files ever opened in the system
> +    increases to 2
> +  */
> +  fd= my_open(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));  
> +  EXPECT_NE(-1, fd);
> +  EXPECT_EQ(1, my_file_opened);
> +  EXPECT_EQ(2, my_file_total_opened);
> +  my_close(fd, MYF(0));
> +  EXPECT_EQ(0, my_file_opened);  
> +  EXPECT_EQ(2, my_file_total_opened);
> +  
> +}
> +
> +TEST_F(My_open_test, TooManyOpenFiles)
> +{
> +  File fd[MAX_NUMBER_OF_FILE_DESCRIPTORS+1];
> +  /*
> +    Attempt to open MAX_NUMBER_OF_FILE_DESCRIPTORS number of files.
> +    NOTE: Couting begins at 0 hence we exclude i==MAX_NUMBER_OF_FILE_DESCRIPTORS
> +  */
> +  int opened_files= 0;
> +  for(unsigned i= 0; i<MAX_NUMBER_OF_FILE_DESCRIPTORS; ++i)
> +  {
> +    fd[i]= my_open(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +    if (fd[i] == -1)
> +      break;
> +    else
> +      ++opened_files;
> +  }
> +  /*
> +    We now have depleted the number of available file descriptors.
> +    Next attempt to open a file will fail.
> +  */
> +  fd[opened_files]= my_open(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_EQ(-1, fd[opened_files]);
> +
> +  /*
> +    Apply my_close() on all file descriptors to make sure all files are closed
> +  */
> +  for(int i= opened_files; i>=0; --i)
> +  {
> +    my_close(fd[i],MYF(0));
> +  }
> +  /*
> +    No more files should be opened.
> +  */
> +  EXPECT_EQ(0, my_file_opened);
> +  EXPECT_EQ(opened_files, my_file_total_opened);
> +}
> +
> +TEST_F(My_open_test, MultipleClose)
> +{
> +  File fd;
> +  fd= my_open(DUMMY_FILE_NAME, O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_NE(-1, fd);
> +  my_close(fd, MYF(0));
> +  my_close(fd, MYF(0));
> +  my_close(fd, MYF(0));
> +  EXPECT_EQ(0, my_file_opened);  
> +  EXPECT_EQ(1, my_file_total_opened);
> +  fd= my_open("/really/dont/exist.bin", O_RDONLY | O_BINARY, MYF(0));
> +  EXPECT_EQ(-1, fd);
> +  my_close(fd, MYF(0));
> +  my_close(fd, MYF(0));
> +  my_close(fd, MYF(0));
> +  EXPECT_EQ(0, my_file_opened);  
> +  EXPECT_EQ(1, my_file_total_opened);
> +}
> +
> 


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


-- 
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
Thread
bzr commit into mysql-trunk branch (kristofer.pettersson:3197) Bug#29071Kristofer Pettersson5 Oct
Re: bzr commit into mysql-trunk branch (kristofer.pettersson:3197)Bug#29071Sergey Vojtovich6 Oct