Various fixes (cleanups, valgrind, makefiles, ...)

server-tools/instance-manager/Makefile.am:
  increased default_monitoring interval
server-tools/instance-manager/guardian.cc:
  some fixes for proper shutdown
server-tools/instance-manager/guardian.h:
  removed init() prototype, as it was never used
server-tools/instance-manager/instance.cc:
  cleanup() function removed
server-tools/instance-manager/instance.h:
  cleanup() prototype removed
server-tools/instance-manager/instance_map.cc:
  Instance_map::cleanup() removed, as instances have nothing to clean up
server-tools/instance-manager/instance_map.h:
  Instance_map::cleanup() prototype removed
server-tools/instance-manager/instance_options.cc:
  added print_argv() function for debug purposes
server-tools/instance-manager/instance_options.h:
  declared print_argv()
server-tools/instance-manager/listener.cc:
  some fixed in listener for proper shutdown
server-tools/instance-manager/log.cc:
  cleanup
server-tools/instance-manager/log.h:
  cleanup
server-tools/instance-manager/manager.cc:
  some comments added
server-tools/instance-manager/mysqlmanager.cc:
  we need to free memory in the very end
server-tools/instance-manager/options.cc:
  fixed default options handling (as they were not working properly), added new method to cleanup Options
server-tools/instance-manager/options.h:
  cleanup() declared
server-tools/instance-manager/thread_registry.cc:
  cleanup
server-tools/instance-manager/user_map.cc:
  missing password file is not a critical error anymore, as IM should be able to work as mysqld_safe only

18 files changed, 85 insertions, 60 deletions

diff --git a/server-tools/instance-manager/Makefile.am b/server-tools/instance-manager/Makefile.am
index fcc4a60c7a8..ff236b10ad5 100644
--- a/server-tools/instance-manager/Makefile.am
+++ b/server-tools/instance-manager/Makefile.am
@@ -33,7 +33,7 @@ liboptions_a_CPPFLAGS= $(CPPFLAGS) \
 	-DDEFAULT_MYSQLD_PATH="$(bindir)/mysqld$(EXEEXT)" \
 	-DDEFAULT_USER="root" \
 	-DDEFAULT_PASSWORD="" \
-	-DDEFAULT_MONITORING_INTERVAL="5" \
+	-DDEFAULT_MONITORING_INTERVAL="20" \
 	-DDEFAULT_PORT="2273" \
 	-DPROTOCOL_VERSION=@PROTOCOL_VERSION@
 
@@ -59,7 +59,7 @@ client_settings.h: Makefile
 	rm -f $(srcdir)/client_settings.h
 	@LN_CP_F@ $(top_srcdir)/sql/client_settings.h $(srcdir)/client_settings.h
 
-bin_PROGRAMS= mysqlmanager
+libexec_PROGRAMS= mysqlmanager
 
 mysqlmanager_SOURCES=	command.cc command.h mysqlmanager.cc \
 			manager.h manager.cc log.h log.cc \
diff --git a/server-tools/instance-manager/guardian.cc b/server-tools/instance-manager/guardian.cc
index e8f9068dbb9..bc05fda1a8f 100644
--- a/server-tools/instance-manager/guardian.cc
+++ b/server-tools/instance-manager/guardian.cc
@@ -48,7 +48,6 @@ Guardian_thread::Guardian_thread(Thread_registry &thread_registry_arg,
   pthread_cond_init(&COND_guardian, 0);
   shutdown_guardian= FALSE;
   is_stopped= FALSE;
-  thread_registry.register_thread(&thread_info);
   init_alloc_root(&alloc, MEM_ROOT_BLOCK_SIZE, 0);
   guarded_instances= NULL;
   starting_instances= NULL;
@@ -60,7 +59,6 @@ Guardian_thread::~Guardian_thread()
   /* delay guardian destruction to the moment when no one needs it */
   pthread_mutex_lock(&LOCK_guardian);
   free_root(&alloc, MYF(0));
-  thread_registry.unregister_thread(&thread_info);
   pthread_mutex_unlock(&LOCK_guardian);
   pthread_mutex_destroy(&LOCK_guardian);
   pthread_cond_destroy(&COND_guardian);
@@ -102,6 +100,8 @@ void Guardian_thread::run()
   LIST *loop;
   struct timespec timeout;
 
+  thread_registry.register_thread(&thread_info);
+
   my_thread_init();
   pthread_mutex_lock(&LOCK_guardian);
 
@@ -110,6 +110,7 @@ void Guardian_thread::run()
   {
     int status= 0;
     loop= guarded_instances;
+
     while (loop != NULL)
     {
       instance= ((GUARD_NODE *) loop->data)->instance;
@@ -167,6 +168,7 @@ void Guardian_thread::run()
     stop_instances();
   is_stopped= TRUE;
   /* now, when the Guardian is stopped we can stop the IM */
+  thread_registry.unregister_thread(&thread_info);
   thread_registry.request_shutdown();
   my_thread_end();
 }
@@ -181,7 +183,7 @@ int Guardian_thread::start()
   while ((instance= iterator.next()))
   {
     if ((instance->options.nonguarded == NULL))
-      if (guard(instance))
+      if (add_instance_to_list(instance, &guarded_instances))
         return 1;
   }
   instance_map->unlock();
diff --git a/server-tools/instance-manager/guardian.h b/server-tools/instance-manager/guardian.h
index d96208247ae..bf96436a636 100644
--- a/server-tools/instance-manager/guardian.h
+++ b/server-tools/instance-manager/guardian.h
@@ -73,7 +73,6 @@ public:
                   uint monitoring_interval_arg);
   ~Guardian_thread();
   void run();
-  int init();
   int start();
   void shutdown();
   void request_stop_instances();
diff --git a/server-tools/instance-manager/instance.cc b/server-tools/instance-manager/instance.cc
index 6c32aa1d815..3ba943c9cd1 100644
--- a/server-tools/instance-manager/instance.cc
+++ b/server-tools/instance-manager/instance.cc
@@ -56,6 +56,7 @@ int Instance::start()
     switch (pid= fork()) {
     case 0:
       execv(options.mysqld_path, options.argv);
+      /* exec never returns */
       exit(1);
     case -1:
       return ER_CANNOT_START_INSTANCE;
@@ -69,12 +70,6 @@ int Instance::start()
 }
 
 
-int Instance::cleanup()
-{
-  return 0;
-}
-
-
 Instance::~Instance()
 {
   pthread_mutex_destroy(&LOCK_instance);
diff --git a/server-tools/instance-manager/instance.h b/server-tools/instance-manager/instance.h
index 29137c749c7..95852542fb4 100644
--- a/server-tools/instance-manager/instance.h
+++ b/server-tools/instance-manager/instance.h
@@ -38,7 +38,6 @@ public:
   bool is_running();
   int start();
   int stop();
-  int cleanup();
 
 public:
   enum { DEFAULT_SHUTDOWN_DELAY= 35 };
diff --git a/server-tools/instance-manager/instance_map.cc b/server-tools/instance-manager/instance_map.cc
index 26181ec56e0..ba373087db4 100644
--- a/server-tools/instance-manager/instance_map.cc
+++ b/server-tools/instance-manager/instance_map.cc
@@ -198,23 +198,6 @@ void Instance_map::complete_initialization()
 }
 
 
-int Instance_map::cleanup()
-{
-  Instance *instance;
-  uint i= 0;
-
-  while (i < hash.records)
-  {
-    instance= (Instance *) hash_element(&hash, i);
-    if (instance->cleanup())
-			return 1;
-    i++;
-  }
-
-	return 0;
-}
-
-
 Instance *
 Instance_map::find(uint instance_number)
 {
diff --git a/server-tools/instance-manager/instance_map.h b/server-tools/instance-manager/instance_map.h
index 05bc4d94908..f2121f7141d 100644
--- a/server-tools/instance-manager/instance_map.h
+++ b/server-tools/instance-manager/instance_map.h
@@ -66,7 +66,6 @@ public:
   Instance *find(uint instance_number);
 
   int flush_instances();
-  int cleanup();
   int lock();
   int unlock();
   int init();
diff --git a/server-tools/instance-manager/instance_options.cc b/server-tools/instance-manager/instance_options.cc
index 0e29dca2422..31a1eafdd00 100644
--- a/server-tools/instance-manager/instance_options.cc
+++ b/server-tools/instance-manager/instance_options.cc
@@ -241,6 +241,16 @@ int Instance_options::add_to_argv(const char* option)
   return 0;
 }
 
+void Instance_options::print_argv()
+{
+  int i;
+  printf("printing out an instance %s argv:\n", instance_name);
+  for (i=0; argv[i] != NULL; i++)
+  {
+    printf("argv: %s\n", argv[i]);
+  }
+}
+
 
 /*
   We execute this function to initialize some options.
diff --git a/server-tools/instance-manager/instance_options.h b/server-tools/instance-manager/instance_options.h
index eae22f840aa..1ed11fc9afa 100644
--- a/server-tools/instance-manager/instance_options.h
+++ b/server-tools/instance-manager/instance_options.h
@@ -50,6 +50,7 @@ public:
   pid_t get_pid();
   void get_pid_filename(char *result);
   int unlink_pidfile();
+  void print_argv();
 
 public:
   /*
diff --git a/server-tools/instance-manager/listener.cc b/server-tools/instance-manager/listener.cc
index 15f57e7e595..8266c0dac4c 100644
--- a/server-tools/instance-manager/listener.cc
+++ b/server-tools/instance-manager/listener.cc
@@ -57,13 +57,11 @@ Listener_thread::Listener_thread(const Listener_thread_args &args) :
   ,total_connection_count(0)
   ,thread_info(pthread_self())
 {
-  thread_registry.register_thread(&thread_info);
 }
 
 
 Listener_thread::~Listener_thread()
 {
-  thread_registry.unregister_thread(&thread_info);
 }
 
 
@@ -82,6 +80,11 @@ void Listener_thread::run()
   enum { LISTEN_BACK_LOG_SIZE = 5 };            // standard backlog size
   int flags;
   int arg= 1;                             /* value to be set by setsockopt */
+  int unix_socket;
+  uint im_port;
+
+  thread_registry.register_thread(&thread_info);
+
   /* I. prepare 'listen' sockets */
 
   int ip_socket= socket(AF_INET, SOCK_STREAM, 0);
@@ -89,8 +92,7 @@ void Listener_thread::run()
   {
     log_error("Listener_thead::run(): socket(AF_INET) failed, %s",
               strerror(errno));
-    thread_registry.request_shutdown();
-    return;
+    goto err;
   }
 
   struct sockaddr_in ip_socket_address;
@@ -104,7 +106,7 @@ void Listener_thread::run()
   }
   else
     im_bind_addr= htonl(INADDR_ANY);
-  uint im_port= options.port_number;
+  im_port= options.port_number;
 
   ip_socket_address.sin_family= AF_INET;
   ip_socket_address.sin_addr.s_addr = im_bind_addr;
@@ -119,16 +121,14 @@ void Listener_thread::run()
   {
     log_error("Listener_thread::run(): bind(ip socket) failed, '%s'",
               strerror(errno));
-    thread_registry.request_shutdown();
-    return;
+   goto err;
   }
 
   if (listen(ip_socket, LISTEN_BACK_LOG_SIZE))
   {
     log_error("Listener_thread::run(): listen(ip socket) failed, %s",
               strerror(errno));
-    thread_registry.request_shutdown();
-    return;
+    goto err;
   }
       /* set the socket nonblocking */
   flags= fcntl(ip_socket, F_GETFL, 0);
@@ -140,13 +140,12 @@ void Listener_thread::run()
   log_info("accepting connections on ip socket");
 
   /*--------------------------------------------------------------*/
-  int unix_socket= socket(AF_UNIX, SOCK_STREAM, 0);
+  unix_socket= socket(AF_UNIX, SOCK_STREAM, 0);
   if (unix_socket == INVALID_SOCKET)
   {
     log_error("Listener_thead::run(): socket(AF_UNIX) failed, %s",
               strerror(errno));
-    thread_registry.request_shutdown();
-    return;
+    goto err;
   }
 
   struct sockaddr_un unix_socket_address;
@@ -169,8 +168,7 @@ void Listener_thread::run()
       log_error("Listener_thread::run(): bind(unix socket) failed, "
                 "socket file name is '%s', error '%s'",
                 unix_socket_address.sun_path, strerror(errno));
-      thread_registry.request_shutdown();
-      return;
+      goto err;
     }
     umask(old_mask);
 
@@ -178,8 +176,7 @@ void Listener_thread::run()
     {
       log_error("Listener_thread::run(): listen(unix socket) failed, %s",
                 strerror(errno));
-      thread_registry.request_shutdown();
-      return;
+      goto err;
     }
 
       /* set the socket nonblocking */
@@ -205,7 +202,15 @@ void Listener_thread::run()
     while (thread_registry.is_shutdown() == false)
     {
       fd_set read_fds_arg= read_fds;
+
+      /*
+        When using valgrind 2.0 this syscall doesn't get kicked off by a
+        signal during shutdown. This results in failing assert
+        (Thread_registry::~Thread_registry). Valgrind 2.2 works fine.
+      */
       int rc= select(n, &read_fds_arg, 0, 0, 0);
+
+
       if (rc == -1 && errno != EINTR)
         log_error("Listener_thread::run(): select() failed, %s",
                   strerror(errno));
@@ -256,6 +261,14 @@ void Listener_thread::run()
   close(unix_socket);
   close(ip_socket);
   unlink(unix_socket_address.sun_path);
+
+  thread_registry.unregister_thread(&thread_info);
+  return;
+
+err:
+  thread_registry.unregister_thread(&thread_info);
+  thread_registry.request_shutdown();
+  return;
 }
 
 
diff --git a/server-tools/instance-manager/log.cc b/server-tools/instance-manager/log.cc
index f89f5e425b8..dbcea3f9c3d 100644
--- a/server-tools/instance-manager/log.cc
+++ b/server-tools/instance-manager/log.cc
@@ -32,8 +32,8 @@
   SYNOPSYS
     log()
 */
-  
-static inline void log(FILE *file,  const char *format, va_list args) 
+
+static inline void log(FILE *file,  const char *format, va_list args)
 {
   /*
     log() should be thread-safe; it implies that we either call fprintf()
diff --git a/server-tools/instance-manager/log.h b/server-tools/instance-manager/log.h
index a1441d5bd71..825d7515513 100644
--- a/server-tools/instance-manager/log.h
+++ b/server-tools/instance-manager/log.h
@@ -17,14 +17,14 @@
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
 
 /*
-  Logging facilities. 
+  Logging facilities.
 
   Two logging streams are supported: error log and info log. Additionally
   libdbug may be used for debug information output.
   ANSI C buffered I/O is used to perform logging.
   Logging is performed via stdout/stder, so one can reopen them to point to
   ordinary files. To initialize loggin environment log_init() must be called.
-  
+
   Rationale:
   - no MYSQL_LOG as it has BIN mode, and not easy to fetch from sql_class.h
   - no constructors/desctructors to make logging available all the time
diff --git a/server-tools/instance-manager/manager.cc b/server-tools/instance-manager/manager.cc
index 6d9ee569d04..b48c020786f 100644
--- a/server-tools/instance-manager/manager.cc
+++ b/server-tools/instance-manager/manager.cc
@@ -127,6 +127,12 @@ void manager(const Options &options)
     pthread_attr_t guardian_thd_attr;
     int rc;
 
+    /*
+       NOTE: Guardian should be shutdowned first. Only then all other threads
+       need to be stopped. This should be done, as guardian is responsible for
+       shutting down the instances, and this is a long operation.
+    */
+
     pthread_attr_init(&guardian_thd_attr);
     pthread_attr_setdetachstate(&guardian_thd_attr, PTHREAD_CREATE_DETACHED);
     rc= pthread_create(&guardian_thd_id, &guardian_thd_attr, guardian,
@@ -160,6 +166,11 @@ void manager(const Options &options)
     making the list. And they in their turn need alarms for timeout suppport.
   */
   guardian_thread.start();
+  /*
+    After the list of guarded instances have been initialized,
+    Guardian should start them.
+  */
+  pthread_cond_signal(&guardian_thread.COND_guardian);
 
   signal(SIGPIPE, SIG_IGN);
 
@@ -176,14 +187,14 @@ void manager(const Options &options)
         /* wake threads waiting for an instance to shutdown */
         pthread_cond_broadcast(&instance_map.pid_cond.COND_pid);
         /* wake guardian */
-        pthread_cond_broadcast(&guardian_thread.COND_guardian);
+        pthread_cond_signal(&guardian_thread.COND_guardian);
       break;
       default:
         if (!guardian_thread.is_stopped)
         {
           guardian_thread.request_stop_instances();
           guardian_thread.shutdown();
-          pthread_cond_broadcast(&guardian_thread.COND_guardian);
+          pthread_cond_signal(&guardian_thread.COND_guardian);
         }
         else
         {
@@ -198,9 +209,6 @@ err:
   /* delete the pid file */
   my_delete(options.pid_file_name, MYF(0));
 
-  /* close permanent connections to the running instances */
-  instance_map.cleanup();
-
   /* free alarm structures */
   end_thr_alarm(1);
   /* don't pthread_exit to kill all threads who did not shut down in time */
diff --git a/server-tools/instance-manager/mysqlmanager.cc b/server-tools/instance-manager/mysqlmanager.cc
index 9ad8ce2c858..bdd7c4ebe78 100644
--- a/server-tools/instance-manager/mysqlmanager.cc
+++ b/server-tools/instance-manager/mysqlmanager.cc
@@ -77,6 +77,8 @@ int main(int argc, char *argv[])
     angel(options);
   }
   manager(options);
+  options.cleanup();
+  my_end(0);
   return 0;
 }
 
diff --git a/server-tools/instance-manager/options.cc b/server-tools/instance-manager/options.cc
index 828a5d7b4d0..db117de03e5 100644
--- a/server-tools/instance-manager/options.cc
+++ b/server-tools/instance-manager/options.cc
@@ -38,6 +38,8 @@ const char *Options::default_mysqld_path= QUOTE(DEFAULT_MYSQLD_PATH);
 const char *Options::bind_address= 0;              /* No default value */
 uint Options::monitoring_interval= DEFAULT_MONITORING_INTERVAL;
 uint Options::port_number= DEFAULT_PORT;
+/* just to declare */
+char **Options::saved_argv;
 
 /*
   List of options, accepted by the instance manager.
@@ -81,7 +83,7 @@ static struct my_option my_long_options[] =
 
   { "port", OPT_PORT, "Port number to use for connections",
     (gptr *) &Options::port_number, (gptr *) &Options::port_number,
-    0, GET_UINT, REQUIRED_ARG, 0, 0, 0, 0, 0, 0 },
+    0, GET_UINT, REQUIRED_ARG, DEFAULT_PORT, 0, 0, 0, 0, 0 },
 
   { "password-file", OPT_PASSWORD_FILE, "Look for Instane Manager users"
                                         " and passwords here.",
@@ -98,7 +100,8 @@ static struct my_option my_long_options[] =
                                             " in seconds.",
                    (gptr *) &Options::monitoring_interval,
                    (gptr *) &Options::monitoring_interval,
-                   0, GET_UINT, REQUIRED_ARG, 0, 0, 0, 0, 0, 0 },
+                   0, GET_UINT, REQUIRED_ARG, DEFAULT_MONITORING_INTERVAL,
+                   0, 0, 0, 0, 0 },
 
   { "run-as-service", OPT_RUN_AS_SERVICE,
     "Daemonize and start angel process.", (gptr *) &Options::run_as_service,
@@ -171,5 +174,11 @@ void Options::load(int argc, char **argv)
 
   if (int rc= handle_options(&argc, &argv, my_long_options, get_one_option))
     exit(rc);
+  Options::saved_argv= argv;
 }
 
+void Options::cleanup()
+{
+  /* free_defaults returns nothing */
+  free_defaults(Options::saved_argv);
+}
diff --git a/server-tools/instance-manager/options.h b/server-tools/instance-manager/options.h
index fc2b44c8b53..8b673bd5fb1 100644
--- a/server-tools/instance-manager/options.h
+++ b/server-tools/instance-manager/options.h
@@ -40,7 +40,10 @@ struct Options
   static uint port_number;
   static const char *bind_address;
 
+  static char **saved_argv;
+
   static void load(int argc, char **argv);
+  void cleanup();
 };
 
 #endif // INCLUDES_MYSQL_INSTANCE_MANAGER_OPTIONS_H
diff --git a/server-tools/instance-manager/thread_registry.cc b/server-tools/instance-manager/thread_registry.cc
index d0bf51f3d61..bf01aa6d7b2 100644
--- a/server-tools/instance-manager/thread_registry.cc
+++ b/server-tools/instance-manager/thread_registry.cc
@@ -197,6 +197,7 @@ void Thread_registry::deliver_shutdown()
     if (info->current_cond)
       pthread_cond_signal(info->current_cond);
   }
+
   pthread_mutex_unlock(&LOCK_thread_registry);
 }
 
diff --git a/server-tools/instance-manager/user_map.cc b/server-tools/instance-manager/user_map.cc
index 8cbceceac7c..f145b611a8d 100644
--- a/server-tools/instance-manager/user_map.cc
+++ b/server-tools/instance-manager/user_map.cc
@@ -128,9 +128,10 @@ int User_map::load(const char *password_file_name)
 
   if ((file= my_fopen(password_file_name, O_RDONLY | O_BINARY, MYF(0))) == 0)
   {
-    log_error("can't open password file %s: errno=%d, %s", password_file_name,
+    /* Probably the password file wasn't specified. Try to leave without it */
+    log_info("can't open password file %s: errno=%d, %s", password_file_name,
               errno, strerror(errno));
-    return 1;
+    return 0;
   }
 
   while (fgets(line, sizeof(line), file))