post-review fixes

server-tools/instance-manager/Makefile.am:
  Removed entry for deleted file
server-tools/instance-manager/buffer.cc:
  cleanup
server-tools/instance-manager/commands.cc:
  cleanup, added missing error handling
server-tools/instance-manager/instance.cc:
  added waitpid in instance_start, added few checks
server-tools/instance-manager/instance_map.cc:
  error handling for hash_init added
server-tools/instance-manager/instance_map.h:
  Extended constructor
server-tools/instance-manager/instance_options.cc:
  made add_option less bulky
server-tools/instance-manager/instance_options.h:
  -
server-tools/instance-manager/listener.cc:
  added missing close, fixed typo
server-tools/instance-manager/manager.cc:
  moved some Instance_map initialization to costructor
server-tools/instance-manager/protocol.cc:
  error handling added
server-tools/instance-manager/protocol.h:
  store_to_string fixed to return a value
server-tools/instance-manager/user_map.cc:
  error handling for hash_init added
server-tools/instance-manager/user_map.h:
  added init() for User map (becouse of the hash_init check)

server-tools/instance-manager/Makefile.am

@@ -80,8 +80,7 @@ mysqlmanager_SOURCES=	mysqlmanager.cc manager.h manager.cc log.h log.cc \
 			instance_map.h instance_map.cc\
 			instance_options.h instance_options.cc \
 			buffer.h buffer.cc parse.cc parse.h \
-			guardian.cc guardian.h common_structures.h \
-                        mysql_manager_error.h
+			guardian.cc guardian.h mysql_manager_error.h
 
 mysqlmanager_LDADD=	liboptions.a \
 			libnet.a \

server-tools/instance-manager/buffer.cc

@@ -77,7 +77,7 @@ int Buffer::append(uint position, const char *string, uint len_arg)
 int Buffer::reserve(uint position, uint len_arg)
 {
   if (position + len_arg >= MAX_BUFFER_SIZE)
-    return 1;
+    goto err;
 
   if (position + len_arg>= buffer_size)
   {
@@ -85,8 +85,13 @@ int Buffer::reserve(uint position, uint len_arg)
                              min(MAX_BUFFER_SIZE,
                                  max((uint) (buffer_size*1.5),
                                      position + len_arg)));
+    if (buffer= NULL)
+      goto err;
     buffer_size= (uint) (buffer_size*1.5);
   }
   return 0;
+
+err:
+  return 1;
 }
 

server-tools/instance-manager/commands.cc

@@ -184,14 +184,17 @@ int Show_instance_status::do_command(struct st_net *net,
     }
 
 
-    my_net_write(net, send_buff.buffer, (uint) position);
+    if (my_net_write(net, send_buff.buffer, (uint) position))
+      goto err;
   }
 
   send_eof(net);
   net_flush(net);
 
-err:
   return 0;
+
+err:
+  return 1;
 }
 
 
@@ -258,7 +261,8 @@ int Show_instance_options::do_command(struct st_net *net,
       goto err;
     store_to_string(&send_buff, (char *) "instance_name", &position);
     store_to_string(&send_buff, (char *) instance_name, &position);
-    my_net_write(net, send_buff.buffer, (uint) position);
+    if (my_net_write(net, send_buff.buffer, (uint) position))
+      goto err;
     if (instance->options.mysqld_path != NULL)
     {
       position= 0;
@@ -266,7 +270,17 @@ int Show_instance_options::do_command(struct st_net *net,
       store_to_string(&send_buff,
                      (char *) instance->options.mysqld_path,
                      &position);
-      my_net_write(net, send_buff.buffer, (uint) position);
+      if (my_net_write(net, send_buff.buffer, (uint) position))
+        goto err;
+    }
+
+    if (instance->options.is_guarded != NULL)
+    {
+      position= 0;
+      store_to_string(&send_buff, (char *) "guarded", &position);
+      store_to_string(&send_buff, "", &position);
+      if (my_net_write(net, send_buff.buffer, (uint) position))
+        goto err;
     }
 
     if (instance->options.mysqld_user != NULL)
@@ -276,7 +290,8 @@ int Show_instance_options::do_command(struct st_net *net,
       store_to_string(&send_buff,
                       (char *) instance->options.mysqld_user,
                       &position);
-      my_net_write(net, send_buff.buffer, (uint) position);
+      if (my_net_write(net, send_buff.buffer, (uint) position))
+        goto err;
     }
 
     if (instance->options.mysqld_password != NULL)
@@ -286,7 +301,8 @@ int Show_instance_options::do_command(struct st_net *net,
       store_to_string(&send_buff,
                       (char *) instance->options.mysqld_password,
                       &position);
-      my_net_write(net, send_buff.buffer, (uint) position);
+      if (my_net_write(net, send_buff.buffer, (uint) position))
+        goto err;
     }
 
     /* loop through the options stored in DYNAMIC_ARRAY */
@@ -302,7 +318,8 @@ int Show_instance_options::do_command(struct st_net *net,
       store_to_string(&send_buff, option_value + 1, &position);
       /* join name and the value into the same option again */
       *option_value= '=';
-      my_net_write(net, send_buff.buffer, (uint) position);
+      if (my_net_write(net, send_buff.buffer, (uint) position))
+        goto err;
     }
   }
 

server-tools/instance-manager/instance.cc

@@ -24,7 +24,7 @@
 #include <my_sys.h>
 #include <signal.h>
 #include <m_string.h>
-
+#include <sys/wait.h>
 
 /*
   The method starts an instance.
@@ -41,11 +41,12 @@
 
 int Instance::start()
 {
+  pid_t pid;
 
   if (!is_running())
   {
     log_info("trying to start instance %s", options.instance_name);
-    switch (fork()) {
+    switch (pid= fork()) {
     case 0:
        if (fork()) /* zombie protection */
          exit(0); /* parent goes bye-bye */
@@ -57,6 +58,7 @@ int Instance::start()
     case -1:
       return ER_CANNOT_START_INSTANCE;
     default:
+      waitpid(pid, NULL, 0);
       return 0;
     }
   }
@@ -77,21 +79,32 @@ int Instance::cleanup()
   return 0;
 }
 
+
 Instance::~Instance()
 {
   pthread_mutex_destroy(&LOCK_instance);
 }
 
+
 bool Instance::is_running()
 {
+  uint port;
+  const char *socket;
+
+  if (options.mysqld_port)
+    port= atoi(strchr(options.mysqld_port, '=') + 1);
+
+  if (options.mysqld_socket)
+    socket= strchr(options.mysqld_socket, '=') + 1;
+
   pthread_mutex_lock(&LOCK_instance);
   if (!is_connected)
   {
     mysql_init(&mysql);
     if (mysql_real_connect(&mysql, LOCAL_HOST, options.mysqld_user,
                            options.mysqld_password,
-                           NullS, atoi(strchr(options.mysqld_port, '=') + 1),
-                           strchr(options.mysqld_socket, '=') + 1, 0))
+                           NullS, port,
+                           socket, 0))
     {
       is_connected= TRUE;
       pthread_mutex_unlock(&LOCK_instance);

server-tools/instance-manager/instance_map.cc

@@ -110,14 +110,26 @@ static int process_option(void * ctx, const char *group, const char *option)
 C_MODE_END
 
 
-Instance_map::Instance_map()
+Instance_map::Instance_map(const char *default_mysqld_path_arg,
+                           const char *default_admin_user_arg,
+                           const char *default_admin_password_arg)
 {
-  hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0,
-            get_instance_key, delete_instance, 0);
+  mysqld_path= default_mysqld_path_arg;
+  user= default_admin_user_arg;
+  password= default_admin_password_arg;
+
   pthread_mutex_init(&LOCK_instance_map, 0);
 }
 
 
+int Instance_map::init()
+{
+  if (hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0,
+                get_instance_key, delete_instance, 0))
+    return 1;
+  return 0;
+}
+
 Instance_map::~Instance_map()
 {
   pthread_mutex_lock(&LOCK_instance_map);

server-tools/instance-manager/instance_map.h

@@ -48,7 +48,7 @@ class Instance_map
     Instance_map *instance_map;
   public:
     Iterator(Instance_map *instance_map_arg) :
-      instance_map(instance_map_arg), current_instance(0)
+     current_instance(0), instance_map(instance_map_arg)
     {}
 
     void go_to_first();
@@ -63,8 +63,11 @@ class Instance_map
   int cleanup();
   int lock();
   int unlock();
+  int init();
 
-  Instance_map();
+  Instance_map(const char *default_mysqld_path_arg,
+               const char *default_admin_user_arg,
+               const char *default_admin_password_arg);
   ~Instance_map();
 
   /* loads options from config files */

server-tools/instance-manager/instance_options.cc

@@ -86,90 +86,58 @@ int Instance_options::complete_initialization(const char *default_path,
 
 int Instance_options::add_option(const char* option)
 {
-  uint elements_count=0;
-  static const char socket[]= "--socket=";
-  static const char port[]= "--port=";
-  static const char datadir[]= "--datadir=";
-  static const char language[]= "--bind-address=";
-  static const char pid_file[]= "--pid-file=";
-  static const char path[]= "--mysqld_path=";
-  static const char user[]= "--admin_user=";
-  static const char password[]= "--admin_password=";
-  static const char guarded[]= "--guarded";
   char *tmp;
-
-  if (!(tmp= strdup_root(&alloc, option)))
-    goto err;
-
- /* To get rid the final zero in a string we subtract 1 from sizeof value */
-  if (strncmp(tmp, socket, sizeof socket - 1) == 0)
-  {
-    mysqld_socket= tmp;
-    goto add_options;
-  }
-
-  if (strncmp(tmp, port, sizeof port - 1) == 0)
-  {
-    mysqld_port= tmp;
-    goto add_options;
-  }
-
-  if (strncmp(tmp, datadir, sizeof datadir - 1) == 0)
-  {
-    mysqld_datadir= tmp;
-    goto add_options;
-  }
-
-  if (strncmp(tmp, language, sizeof language - 1) == 0)
+  enum { SAVE_VALUE= 1, SAVE_WHOLE, SAVE_WHOLE_AND_ADD };
+  struct selected_options_st
   {
-    mysqld_bind_address= tmp;
-    goto add_options;
-  }
-
-  if (strncmp(tmp, pid_file, sizeof pid_file - 1) == 0)
+    const char *name;
+    uint length;
+    const char **value;
+    uint type;
+  } options[]=
   {
-    mysqld_pid_file= tmp;
-    goto add_options;
-  }
+    {"--socket=", 9, &mysqld_socket, SAVE_WHOLE_AND_ADD},
+    {"--port=", 7, &mysqld_port, SAVE_WHOLE_AND_ADD},
+    {"--datadir=", 10, &mysqld_datadir, SAVE_WHOLE_AND_ADD},
+    {"--bind-address=", 15, &mysqld_bind_address, SAVE_WHOLE_AND_ADD},
+    {"--pid-file=", 11, &mysqld_pid_file, SAVE_WHOLE_AND_ADD},
+    {"--mysqld_path=", 14, &mysqld_path, SAVE_VALUE},
+    {"--admin_user=", 13, &mysqld_user, SAVE_VALUE},
+    {"--admin_password=", 17, &mysqld_password, SAVE_VALUE},
+    {"--guarded", 9, &is_guarded, SAVE_WHOLE},
+    {NULL, 0, NULL, 0}
+  };
+  struct selected_options_st *selected_options;
 
-  /*
-    We don't need a prefix in the next three optios.
-    We also don't need to add them to argv array =>
-    return instead of goto.
-  */
-
-  if (strncmp(tmp, path, sizeof path - 1) == 0)
-  {
-    mysqld_path= strchr(tmp, '=') + 1;
-    return 0;
-  }
-
-  if (strncmp(tmp, user, sizeof user - 1) == 0)
-  {
-    mysqld_user= strchr(tmp, '=') + 1;
-    return 0;
-  }
-
-  if (strncmp(tmp, password, sizeof password - 1) == 0)
-  {
-    mysqld_password= strchr(tmp, '=') + 1;
-    return 0;
-  }
+  if (!(tmp= strdup_root(&alloc, option)))
+    goto err;
 
-  if (strncmp(tmp, guarded, sizeof guarded - 1) == 0)
-  {
-    is_guarded= tmp;
-    return 0;
-  }
+   for (selected_options= options; selected_options->name; selected_options++)
+   {
+     if (!strncmp(tmp, selected_options->name, selected_options->length))
+       switch(selected_options->type){
+       case SAVE_WHOLE_AND_ADD:
+         *(selected_options->value)= tmp;
+         insert_dynamic(&options_array,(gptr) &tmp);
+         return 0;
+       case SAVE_VALUE:
+         *(selected_options->value)= strchr(tmp, '=') + 1;
+         return 0;
+       case SAVE_WHOLE:
+         *(selected_options->value)= tmp;
+         return 0;
+       defaut:
+         break;
+       }
+   }
 
-add_options:
-  insert_dynamic(&options_array,(gptr) &tmp);
   return 0;
 
 err:
   return 1;
 }
 
+
 int Instance_options::add_to_argv(const char* option)
 {
   DBUG_ASSERT(filled_default_options < MAX_NUMBER_OF_DEFAULT_OPTIONS);
@@ -191,7 +159,8 @@ int Instance_options::init(const char *instance_name_arg)
 
   init_alloc_root(&alloc, MEM_ROOT_BLOCK_SIZE, 0);
 
-  my_init_dynamic_array(&options_array, sizeof(char *), 0, 32);
+  if (my_init_dynamic_array(&options_array, sizeof(char *), 0, 32))
+      goto err;
 
   if (!(instance_name= strmake_root(&alloc, (char *) instance_name_arg,
                                   instance_name_len)))

server-tools/instance-manager/listener.cc

@@ -132,7 +132,7 @@ void Listener_thread::run()
       /* set the socket nonblocking */
   flags= fcntl(ip_socket, F_GETFL, 0);
   fcntl(ip_socket, F_SETFL, flags | O_NONBLOCK);
-    /* make shure that instances won't be listening our sockets */
+    /* make sure that instances won't be listening our sockets */
   flags= fcntl(ip_socket, F_GETFD, 0);
   fcntl(ip_socket, F_SETFD, flags | FD_CLOEXEC);
 
@@ -184,7 +184,7 @@ void Listener_thread::run()
       /* set the socket nonblocking */
     flags= fcntl(unix_socket, F_GETFL, 0);
     fcntl(unix_socket, F_SETFL, flags | O_NONBLOCK);
-      /* make shure that instances won't be listening our sockets */
+      /* make sure that instances won't be listening our sockets */
     flags= fcntl(unix_socket, F_GETFD, 0);
     fcntl(unix_socket, F_SETFD, flags | FD_CLOEXEC);
   }
@@ -253,6 +253,7 @@ void Listener_thread::run()
   log_info("Listener_thread::run(): shutdown requested, exiting...");
 
   close(unix_socket);
+  close(ip_socket);
   unlink(unix_socket_address.sun_path);
 }
 

server-tools/instance-manager/manager.cc

@@ -65,7 +65,9 @@ void manager(const Options &options)
   */
 
   User_map user_map;
-  Instance_map instance_map;
+  Instance_map instance_map(options.default_mysqld_path,
+                            options.default_admin_user,
+                            options.default_admin_password);
   Guardian_thread guardian_thread(thread_registry,
                                   &instance_map,
                                   options.monitoring_interval);
@@ -73,16 +75,10 @@ void manager(const Options &options)
   Listener_thread_args listener_args(thread_registry, options, user_map,
                                      instance_map);
 
-  instance_map.mysqld_path= options.default_mysqld_path;
-  instance_map.user= options.default_admin_user;
-  instance_map.password= options.default_admin_password;
   instance_map.guardian= &guardian_thread;
 
-
-  if (instance_map.load())
-    return;
-
-  if (user_map.load(options.password_file_name))
+  if (instance_map.init() || user_map.init() || instance_map.load() ||
+      user_map.load(options.password_file_name))
     return;
 
   /* write pid file */

server-tools/instance-manager/protocol.cc

@@ -99,16 +99,22 @@ char *net_store_length(char *pkg, uint length)
 }
 
 
-void store_to_string(Buffer *buf, const char *string, uint *position)
+int store_to_string(Buffer *buf, const char *string, uint *position)
 {
   uint currpos;
   uint string_len;
 
   string_len= strlen(string);
-  buf->reserve(*position, 2);
+  if (buf->reserve(*position, 2))
+    goto err;
   currpos= (net_store_length(buf->buffer + *position, string_len) - buf->buffer);
-  buf->append(currpos, string, string_len);
+  if (buf->append(currpos, string, string_len))
+    goto err;
   *position= *position + string_len + (currpos - *position);
+
+  return 0;
+err:
+  return 1;
 }
 
 
@@ -134,7 +140,8 @@ int send_fields(struct st_net *net, LIST *fields)
 
   /* send the number of fileds */
   net_store_length(small_buff, (uint) list_length(fields));
-  my_net_write(net, small_buff, (uint) 1);
+  if (my_net_write(net, small_buff, (uint) 1))
+    goto err;
 
   while (tmp)
   {
@@ -147,7 +154,8 @@ int send_fields(struct st_net *net, LIST *fields)
     store_to_string(&send_buff, (char *) "", &position); /* table name alias */
     store_to_string(&send_buff, field->name, &position); /* column name */
     store_to_string(&send_buff, field->name, &position); /* column name alias */
-    send_buff.reserve(position, 12);
+    if (send_buff.reserve(position, 12))
+      goto err;
     send_buff.buffer[position++]= 12;
     int2store(send_buff.buffer + position, 1); /* charsetnr */
     int4store(send_buff.buffer + position + 2, field->length); /* field length */
@@ -162,7 +170,7 @@ int send_fields(struct st_net *net, LIST *fields)
     tmp= rest(tmp);
   }
 
-  if ( my_net_write(net, eof_buff, 1))
+  if (my_net_write(net, eof_buff, 1))
     goto err;
   return 0;
 

server-tools/instance-manager/protocol.h

@@ -36,7 +36,7 @@ int send_fields(struct st_net *net, LIST *fields);
 
 char *net_store_length(char *pkg, uint length);
 
-void store_to_string(Buffer *buf, const char *string, uint *position);
+int store_to_string(Buffer *buf, const char *string, uint *position);
 
 int send_eof(struct st_net *net);
 

server-tools/instance-manager/user_map.cc

@@ -93,13 +93,16 @@ static void delete_user(void *u)
 C_MODE_END
 
 
-User_map::User_map()
+int User_map::init()
 {
   enum { START_HASH_SIZE = 16 };
-  hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0,
-            get_user_key, delete_user, 0);
+  if (hash_init(&hash, default_charset_info, START_HASH_SIZE, 0, 0,
+      get_user_key, delete_user, 0))
+    return 1;
+  return 0;
 }
 
+
 User_map::~User_map()
 {
   hash_free(&hash);
@@ -134,7 +137,8 @@ int User_map::load(const char *password_file_name)
   while (fgets(line, sizeof(line), file))
   {
     /* skip comments and empty lines */
-    if (line[0] == '#' || line[0] == '\n' && line[1] == '\0')
+    if (line[0] == '#' || line[0] == '\n' &&
+        (line[1] == '\0' || line[1] == '\r'))
       continue;
     if ((user= new User) == 0)
       goto done;

server-tools/instance-manager/user_map.h

@@ -31,9 +31,9 @@
 class User_map
 {
 public:
-  User_map();
   ~User_map();
 
+  int init();
   int load(const char *password_file_name);
   int authenticate(const char *user_name, uint length,
                    const char *scrambled_password,