Re: bk commit into 5.1 tree (thek:1.2528) BUG#28012

[Alexander Nozdrin]

Hi Kristofer,

thanks for working on this. As we discussed it on IRC and email,
the patch is Ok to push after minor changes.

On Monday 23 July 2007 17:05, kpettersson@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of thek. When thek does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-07-23 15:05:50+02:00, thek@adventure.(none) +6 -0
>   Bug#28012 Patch : IM crashes instead of reporting an error when mysqldpath is bad
>     
>   On the windows platform, if an instance object failed to initialize during
>   program start, the instance manager would crash.
>   This could happen if an incorrect mysqld path was supplied in the 
>   defaults configuration file.
>   The patch prevents the program from crashing and makes it show an
>   error message instead.
> 
>   mysql-test/r/im_options.result@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +6 -4
>     - Options have changed names.
> 
>   server-tools/instance-manager/instance.cc@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +14 -22
>     - Added code to verify that the instance object was initialized
>       before any attempt is made to start the associated process.
>     - Instance::complete_initialization method will now return TRUE
>       on an error during instance initialization.
> 
>   server-tools/instance-manager/instance_options.cc@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +7 -6
>     - Parsed result byte sequence from executed process doesn't contain a new line
>       character anymore.
> 
>   server-tools/instance-manager/parse_output.cc@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +342 -63
>     - 'popen' didn't behave as intended on the windows platform.
>     - The function parse_output_and_get_value was completly rewritten to correct the
>       error in the windows built and to be more easily maintained across platforms.
>      
> 
>   server-tools/instance-manager/parse_output.h@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +11 -5
>     - 'popen' didn't behave as intended on the windows platform.
>     - The function parse_output_and_get_value was completly rewritten to correct the
>       error in the windows built and to be more easily maintained across platforms.
>      
> 
>   server-tools/instance-manager/portability.h@stripped, 2007-07-23 15:05:48+02:00,
> thek@adventure.(none) +5 -0
>     - Add more portability constants for convenience.
> 
> diff -Nrup a/mysql-test/r/im_options.result b/mysql-test/r/im_options.result
> --- a/mysql-test/r/im_options.result	2007-02-18 13:45:23 +01:00
> +++ b/mysql-test/r/im_options.result	2007-07-23 15:05:48 +02:00
> @@ -37,9 +37,10 @@ log-slow-queries	option_value
>  language	option_value
>  character-sets-dir	option_value
>  basedir	option_value
> +shutdown-delay	option_value
>  skip-stack-trace	option_value
> -skip-innodb	option_value
> -skip-ndbcluster	option_value
> +loose-skip-innodb	option_value
> +loose-skip-ndbcluster	option_value
>  nonguarded	option_value
>  log-output	option_value
>  SET mysqld2.server_id = 2;
> @@ -57,9 +58,10 @@ log-slow-queries	option_value
>  language	option_value
>  character-sets-dir	option_value
>  basedir	option_value
> +shutdown-delay	option_value
>  skip-stack-trace	option_value
> -skip-innodb	option_value
> -skip-ndbcluster	option_value
> +loose-skip-innodb	option_value
> +loose-skip-ndbcluster	option_value
>  nonguarded	option_value
>  log-output	option_value
>  server_id	option_value
> diff -Nrup a/server-tools/instance-manager/instance.cc
> b/server-tools/instance-manager/instance.cc
> --- a/server-tools/instance-manager/instance.cc	2007-02-23 12:13:48 +01:00
> +++ b/server-tools/instance-manager/instance.cc	2007-07-23 15:05:48 +02:00
> @@ -524,24 +524,17 @@ bool Instance::init(const LEX_STRING *na
>  
>  
>  /**
> -  Complete instance options initialization.
> +  @brief Complete instance options initialization.
>  
> -  SYNOPSIS
> -    complete_initialization()
> -
> -  RETURN
> -    FALSE - ok
> -    TRUE  - error
> +  @return Error status.
> +    @retval FALSE ok
> +    @retval TRUE error
>  */
>  
>  bool Instance::complete_initialization()
>  {
>    configured= ! options.complete_initialization();
> -  return FALSE;
> -  /*
> -    TODO: return actual status (from
> -    Instance_options::complete_initialization()) here.
> -  */
> +  return !configured;
>  }
>  
>  /**************************************************************************
> @@ -644,24 +637,23 @@ bool Instance::is_mysqld_running()
>  
>  
>  /**
> -  Start mysqld.
> -
> -  SYNOPSIS
> -    start_mysqld()
> +  @brief Start mysqld.
>  
> -  DESCRIPTION
> -    Reset flags and start Instance Monitor thread, which will start mysqld.
> +  Reset flags and start Instance Monitor thread, which will start mysqld.
>  
> -    MT-NOTE: instance must be locked before calling the operation.
> +  @note Instance must be locked before calling the operation.
>  
> -  RETURN
> -    FALSE - ok
> -    TRUE  - could not start instance
> +  @return Error status code
> +    @retval FALSE Ok
> +    @retval TRUE Could not start instance
>  */
>  
>  bool Instance::start_mysqld()
>  {
>    Instance_monitor *instance_monitor;
> +
> +  if (!configured)
> +    return TRUE;
>  
>    /*
>      Prepare instance to start Instance Monitor thread.
> diff -Nrup a/server-tools/instance-manager/instance_options.cc
> b/server-tools/instance-manager/instance_options.cc
> --- a/server-tools/instance-manager/instance_options.cc	2007-05-10 11:59:26 +02:00
> +++ b/server-tools/instance-manager/instance_options.cc	2007-07-23 15:05:48 +02:00
> @@ -156,7 +156,8 @@ int Instance_options::get_default_option
>      goto err;
>  
>    /* +2 eats first "--" from the option string (E.g. "--datadir") */
> -  rc= parse_output_and_get_value((char*) cmd.buffer, option_name + 2,
> +  rc= parse_output_and_get_value((char*) cmd.buffer,
> +                                 option_name + 2, strlen(option_name + 2),
>                                   result, result_len, GET_VALUE);
>  err:
>    return rc;
> @@ -194,8 +195,8 @@ bool Instance_options::fill_instance_ver
>  
>    bzero(result, MAX_VERSION_LENGTH);
>  
> -  if (parse_output_and_get_value((char*) cmd.buffer, "Ver", result,
> -                                 MAX_VERSION_LENGTH, GET_LINE))
> +  if (parse_output_and_get_value((char*) cmd.buffer, STRING_WITH_LEN("Ver"),
> +                                 result, MAX_VERSION_LENGTH, GET_LINE))
>    {
>      log_error("Failed to get version of '%s': unexpected output.",
>                (const char *) mysqld_path.str);
> @@ -206,8 +207,7 @@ bool Instance_options::fill_instance_ver
>  
>    {
>      char *start;
> -    /* chop the newline from the end of the version string */
> -    result[strlen(result) - NEWLINE_LEN]= '\0';
> +
>      /* trim leading whitespaces */
>      start= result;
>      while (my_isspace(default_charset_info, *start))
> @@ -255,7 +255,8 @@ bool Instance_options::fill_mysqld_real_
>  
>    bzero(result, FN_REFLEN);
>  
> -  if (parse_output_and_get_value((char*) cmd.buffer, "Usage: ",
> +  if (parse_output_and_get_value((char*) cmd.buffer,
> +                                 STRING_WITH_LEN("Usage: "),
>                                   result, FN_REFLEN,
>                                   GET_LINE))
>    {
> diff -Nrup a/server-tools/instance-manager/parse_output.cc
> b/server-tools/instance-manager/parse_output.cc
> --- a/server-tools/instance-manager/parse_output.cc	2006-12-23 20:19:48 +01:00
> +++ b/server-tools/instance-manager/parse_output.cc	2007-07-23 15:05:48 +02:00
> @@ -24,6 +24,13 @@
>  #include "parse.h"
>  #include "portability.h"
>  
> +/**************************************************************************
> +  Private module implementation.
> +**************************************************************************/
> +
> +namespace { /* no-indent */
> +
> +/*************************************************************************/
>  
>  void trim_space(const char **text, uint *word_len)
>  {
> @@ -39,90 +46,362 @@ void trim_space(const char **text, uint 
>    *word_len= (end - start)+1;
>  }
>  
> -/*
> -  Parse output of the given command
> +/*************************************************************************/
>  
> -  SYNOPSIS
> -    parse_output_and_get_value()
> -
> -    command          the command to execue with popen.
> -    word             the word to look for (usually an option name)
> -    result           the buffer to store the next word (option value)
> -    input_buffer_len self-explanatory
> -    flag             this equals to GET_LINE if we want to get all the line after
> -                     the matched word and GET_VALUE otherwise.
> -
> -  DESCRIPTION
> -
> -    Parse output of the "command". Find the "word" and return the next one
> -    if flag is GET_VALUE. Return the rest of the parsed string otherwise.
> -
> -  RETURN
> -    0 - ok, the word has been found
> -    1 - error occured or the word is not found
> +/**
> +  @brief A facade to the internal workings of optaining the output from an
> +  executed system process.
>  */
>  
> -int parse_output_and_get_value(const char *command, const char *word,
> -                               char *result, size_t input_buffer_len,
> -                               uint flag)
> +class Mysqld_output_parser
>  {
> -  FILE *output;
> -  uint wordlen;
> -  /* should be enough to store the string from the output */
> -  enum { MAX_LINE_LEN= 512 };
> -  char linebuf[MAX_LINE_LEN];
> -  int rc= 1;
> +public:
> +  Mysqld_output_parser()
> +  { }
> +
> +  virtual ~Mysqld_output_parser()
> +  { }
> +
> +public:
> +  bool parse(const char *command,
> +             const char *option_name_str,
> +             uint option_name_length,
> +             char *option_value_buf,
> +             size_t option_value_buf_size,
> +             enum_option_type option_type);
> +
> +protected:
> +  /**
> +    @brief Run a process and attach stdout- and stdin-pipes to it.
> +
> +    @param command The path to the process to be executed
> +
> +    @return Error status.
> +      @retval TRUE An error occurred
> +      @retval FALSE Operation was a success
> +  */
>  
> -  wordlen= strlen(word);
> +  virtual bool run_command(const char *command)= 0;
>  
> -  /*
> -    Successful return of popen does not tell us whether the command has been
> -    executed successfully: if the command was not found, we'll get EOF
> -    when reading the output buffer below.
> +
> +  /**
> +    @brief Read a sequence of bytes from the executed process' stdout pipe.
> +
> +    The sequence is terminated by either '\0', LF or CRLF tokens. The
> +    terminating token is excluded from the result.
> +
> +    @param line_buffer A pointer to a character buffer
> +    @param line_buffer_size The size of the buffer in bytes
> +
> +    @return Error status.
> +      @retval TRUE An error occured
> +      @retval FALSE Operation was a success
>    */
> -  if (!(output= popen(command, "r")))
> -    goto err;
>  
> -  /*
> -    We want fully buffered stream. We also want system to
> -    allocate appropriate buffer.
> +  virtual bool read_line(char *line_buffer,
> +                         uint line_buffer_size)= 0;
> +
> +
> +  /**
> +    @brief Release any resources needed after a execution and parsing.
>    */
> -  setvbuf(output, NULL, _IOFBF, 0);
>  
> -  while (fgets(linebuf, sizeof(linebuf) - 1, output))
> +  virtual bool cleanup()= 0;
> +};
> +
> +/*************************************************************************/
> +
> +bool Mysqld_output_parser::parse(const char *command,
> +                                 const char *option_name_str,
> +                                 uint option_name_length,
> +                                 char *option_value_buf,
> +                                 size_t option_value_buf_size,
> +                                 enum_option_type option_type)
> +{
> +  /* should be enough to store the string from the output */
> +  const int LINE_BUFFER_SIZE= 512;
> +  char line_buffer[LINE_BUFFER_SIZE];
> +
> +  if (run_command(command))
> +    return TRUE;
> +
> +  while (true)
>    {
> +    if (read_line(line_buffer, LINE_BUFFER_SIZE))
> +    {
> +      cleanup();
> +      return TRUE;
> +    }
> +
>      uint found_word_len= 0;
> -    char *linep= linebuf;
> +    char *linep= line_buffer;
> +
> +    line_buffer[sizeof(line_buffer) - 1]= '\0';        /* safety */
> +
> +    /* Find the word(s) we are looking for in the line. */
> +
> +    linep= strstr(linep, option_name_str);
>  
> -    linebuf[sizeof(linebuf) - 1]= '\0';        /* safety */
> +    if (!linep)
> +      continue;
>  
> -    /*
> -      Find the word(s) we are looking for in the line
> -    */
> -    if ((linep= strstr(linep, word)))
> +    linep+= option_name_length;
> +
> +    switch (option_type)
>      {
> -      /*
> -        If we have found our word(s), then move linep past the word(s)
> -      */
> -      linep+= wordlen;
> -      if (flag & GET_VALUE)
> +    case GET_VALUE:
> +      trim_space((const char**) &linep, &found_word_len);
> +
> +      if (option_value_buf_size <= found_word_len)
>        {
> -        trim_space((const char**) &linep, &found_word_len);
> -        if (input_buffer_len <= found_word_len)
> -          goto err;
> -        strmake(result, linep, found_word_len);
> +        cleanup();
> +        return TRUE;
>        }
> -      else         /* currently there are only two options */
> -        strmake(result, linep, input_buffer_len - 1);
> -      rc= 0;
> +
> +      strmake(option_value_buf, linep, found_word_len);
> +
> +      break;
> +
> +    case GET_LINE:
> +      strmake(option_value_buf, linep, option_value_buf_size - 1);
> +
>        break;
>      }
> +
> +    cleanup();
> +
> +    return FALSE;
> +  }
> +}
> +
> +/**************************************************************************
> +  Platform-specific implementation: UNIX.
> +**************************************************************************/
> +
> +#ifndef __WIN__
> +
> +class Mysqld_output_parser_unix : public Mysqld_output_parser
> +{
> +public:
> +  Mysqld_output_parser_unix() :
> +    m_stdout(NULL)
> +  { }
> +
> +protected:
> +  virtual bool run_command(const char *command);
> +
> +  virtual bool read_line(char *line_buffer,
> +                         uint line_buffer_size);
> +
> +  virtual bool cleanup();
> +
> +private:
> +  FILE *m_stdout;
> +};
> +
> +bool Mysqld_output_parser_unix::run_command(const char *command)
> +{
> +  if (!(m_stdout= popen(command, "r")))
> +    return TRUE;
> +
> +  /*
> +    We want fully buffered stream. We also want system to allocate
> +    appropriate buffer.
> +  */
> +
> +  setvbuf(m_stdout, NULL, _IOFBF, 0);
> +
> +  return FALSE;
> +}
> +
> +bool Mysqld_output_parser_unix::read_line(char *line_buffer,
> +                                          uint line_buffer_size)
> +{
> +  char *retbuff = fgets(line_buffer, line_buffer_size, m_stdout);
> +  /* Remove any tailing new line charaters */
> +  if (line_buffer[line_buffer_size-1] == LF)
> +   line_buffer[line_buffer_size-1]= '\0';
> +  return (retbuff == NULL);
> +}
> +
> +bool Mysqld_output_parser_unix::cleanup()
> +{
> +  if (m_stdout)
> +    pclose(m_stdout);
> +
> +  return FALSE;
> +}
> +
> +#else /* Windows */
> +
> +/**************************************************************************
> +  Platform-specific implementation: Windows.
> +**************************************************************************/
> +
> +class Mysqld_output_parser_win : public Mysqld_output_parser
> +{
> +public:
> +  Mysqld_output_parser_win() :
> +      m_internal_buffer(NULL),
> +      m_internal_buffer_offset(0),
> +      m_internal_buffer_size(0)
> +  { }
> +
> +protected:
> +  virtual bool run_command(const char *command);
> +  virtual bool read_line(char *line_buffer,
> +                         uint line_buffer_size);
> +  virtual bool cleanup();
> +
> +private:
> +  HANDLE m_h_child_stdout_wr;
> +  HANDLE m_h_child_stdout_rd;
> +  uint m_internal_buffer_offset;
> +  uint m_internal_buffer_size;
> +  char *m_internal_buffer;
> +};
> +
> +bool Mysqld_output_parser_win::run_command(const char *command)
> +{
> +  BOOL op_status;
> +
> +  SECURITY_ATTRIBUTES sa_attr;
> +  sa_attr.nLength= sizeof(SECURITY_ATTRIBUTES);
> +  sa_attr.bInheritHandle= TRUE;
> +  sa_attr.lpSecurityDescriptor= NULL;
> +
> +  op_status= CreatePipe(&m_h_child_stdout_rd,
> +                        &m_h_child_stdout_wr,
> +                        &sa_attr,
> +                        0 /* Use system-default buffer size. */);
> +
> +  if (!op_status)
> +    return TRUE;
> +
> +  SetHandleInformation(m_h_child_stdout_rd, HANDLE_FLAG_INHERIT, 0);
> +
> +  STARTUPINFO si_start_info;
> +  ZeroMemory(&si_start_info, sizeof(STARTUPINFO));
> +  si_start_info.cb= sizeof(STARTUPINFO);
> +  si_start_info.hStdError= m_h_child_stdout_wr;
> +  si_start_info.hStdOutput= m_h_child_stdout_wr;
> +  si_start_info.dwFlags|= STARTF_USESTDHANDLES;
> +
> +  PROCESS_INFORMATION pi_proc_info;
> +
> +  op_status= CreateProcess(NULL,           /* Application name. */
> +                           (char*)command, /* Command line. */
> +                           NULL,           /* Process security attributes. */
> +                           NULL,           /* Primary thread security attr.*/
> +                           TRUE,           /* Handles are inherited. */
> +                           0,              /* Creation flags. */
> +                           NULL,           /* Use parent's environment. */
> +                           NULL,           /* Use parent's curr. directory. */
> +                           &si_start_info, /* STARTUPINFO pointer. */
> +                           &pi_proc_info); /* Rec. PROCESS_INFORMATION. */
> +
> +  if (!retval)
> +  {
> +    CloseHandle(m_h_child_stdout_rd);
> +    CloseHandle(m_h_child_stdout_wr);
> +
> +    return TRUE;
>    }
>  
> -  /* we are not interested in the termination status */
> -  pclose(output);
> +  /* Close unnessary handles. */
> +
> +  CloseHandle(pi_proc_info.hProcess);
> +  CloseHandle(pi_proc_info.hThread);
>  
> -err:
> -  return rc;
> +  return FALSE;
>  }
>  
> +bool Mysqld_output_parser_win::read_line(char *line_buffer,
> +                                         uint line_buffer_size)
> +{
> +  DWORD dw_read_count= m_internal_buffer_size;
> +  bzero(line_buffer,line_buffer_size);
> +  char *buff_ptr= line_buffer;
> +  char ch;
> +
> +  while (buff_ptr - line_buffer < line_buffer_size)
> +  {
> +    do
> +    {
> +      ReadFile(m_h_child_stdout_rd, &ch,
> +               1, &dw_read_count, NULL);
> +    } while ((ch == CR || ch == LF) && buff_ptr == line_buffer);
> +
> +    if (dw_read_count == 0)
> +      return TRUE;
> +
> +    if (ch == CR || ch == LF)
> +      break;
> +
> +    *buff_ptr++ = ch;
> +  }
> +
> +  return FALSE;
> +}
> +
> +bool Mysqld_output_parser_win::cleanup()
> +{
> +  /* Close all handles. */
> +
> +  CloseHandle(m_h_child_stdout_wr);
> +  CloseHandle(m_h_child_stdout_rd);
> +
> +  return FALSE;
> +}
> +#endif
> +
> +/*************************************************************************/
> +
> +} /* End of private module implementation. */
> +
> +/*************************************************************************/
> +
> +/**
> +  @brief Parse output of the given command
> +
> +  @param      command               The command to execute.
> +  @param      option_name_str       Option name.
> +  @param      option_name_length    Length of the option name.
> +  @param[out] option_value_buf      The buffer to store option value.
> +  @param      option_value_buf_size Size of the option value buffer.
> +  @param      option_type           Type of the option:
> +                                      - GET_LINE if we want to get all the
> +                                        line after the option name;
> +                                      - GET_VALUE otherwise.
> +
> +  Execute the process by running "command". Find the "option name" and
> +  return the next word if "option_type" is GET_VALUE. Return the rest of
> +  the parsed string otherwise.
> +
> +  @note This function has a separate windows implementation.
> +
> +  @return The error status.
> +    @retval FALSE Ok, the option name has been found.
> +    @retval TRUE Error occured or the option name is not found.
> +*/
> +
> +bool parse_output_and_get_value(const char *command,
> +                                const char *option_name_str,
> +                                uint option_name_length,
> +                                char *option_value_buf,
> +                                size_t option_value_buf_size,
> +                                enum_option_type option_type)
> +{
> +#ifndef __WIN__
> +  Mysqld_output_parser_unix parser;
> +#else /* __WIN__ */
> +  Mysqld_output_parser_win parser;
> +#endif
> +
> +  return parser.parse(command,
> +                      option_name_str,
> +                      option_name_length,
> +                      option_value_buf,
> +                      option_value_buf_size,
> +                      option_type);
> +}
> diff -Nrup a/server-tools/instance-manager/parse_output.h
> b/server-tools/instance-manager/parse_output.h
> --- a/server-tools/instance-manager/parse_output.h	2006-12-23 20:19:48 +01:00
> +++ b/server-tools/instance-manager/parse_output.h	2007-07-23 15:05:48 +02:00
> @@ -17,11 +17,17 @@
>  
>  #include <my_global.h>
>  
> -#define GET_VALUE 1
> -#define GET_LINE  2
> +enum enum_option_type
> +{
> +  GET_VALUE = 1,
> +  GET_LINE
> +};
>  
> -int parse_output_and_get_value(const char *command, const char *word,
> -                               char *result, size_t input_buffer_len,
> -                               uint flag);
> +bool parse_output_and_get_value(const char *command,
> +                                const char *option_name_str,
> +                                uint option_name_length,
> +                                char *option_value_buf,
> +                                size_t option_value_buf_size,
> +                                enum_option_type option_type);
>  
>  #endif /* INCLUDES_MYSQL_INSTANCE_MANAGER_PARSE_OUTPUT_H */
> diff -Nrup a/server-tools/instance-manager/portability.h
> b/server-tools/instance-manager/portability.h
> --- a/server-tools/instance-manager/portability.h	2006-12-31 01:06:33 +01:00
> +++ b/server-tools/instance-manager/portability.h	2007-07-23 15:05:48 +02:00
> @@ -48,10 +48,15 @@ typedef int pid_t;
>  #define NEWLINE "\r\n"
>  #define NEWLINE_LEN 2
>  
> +const char CR = '\r';
> +const char LF = '\n';
> +
>  #else /* ! __WIN__ */
>  
>  #define NEWLINE "\n"
>  #define NEWLINE_LEN 1
> +
> +const char LF = '\n';
>  
>  #endif /* __WIN__ */
>  
> 

-- 
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com