mwl#59 - windows installer.

Address Monty's review comments

sql/mysql_install_db.cc

+/* Copyright (C) 2010-2011 Monty Program Ab & Vladislav Vaintroub
 
+   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, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
+
+/*
+  mysql_install_db creates a new database instance (optionally as service)
+  on Windows.
+*/
 #define DONT_DEFINE_VOID
 #include <my_global.h>
 #include <my_getopt.h>
 #include <my_sys.h>
 #include <m_string.h>
 
-#include <stdlib.h>
-#include <stdio.h>
 #include <windows.h>
 #include <assert.h>
 #include <shellapi.h>
@@ -14,7 +30,8 @@
 #include <aclapi.h>
 
 #define USAGETEXT \
-"mysql_install_db.exe  Ver 1.42 for Windows\n" \
+"mysql_install_db.exe  Ver 1.00 for Windows\n" \
+"Copyright (C) 2010-2011 Monty Program Ab & Vladislav Vaintroub\n" \
 "This software comes with ABSOLUTELY NO WARRANTY. This is free software,\n" \
 "and you are welcome to modify and redistribute it under the GPL v2 license\n" \
 "Usage: mysql_install_db.exe [OPTIONS]\n" \
@@ -22,9 +39,9 @@
 
 extern "C" const char mysql_bootstrap_sql[];
 
-char default_os_user[] = "NT AUTHORITY\\NetworkService";
+char default_os_user[]= "NT AUTHORITY\\NetworkService";
 static int create_db_instance();
-static uint opt_verbose, opt_silent;
+static uint opt_silent;
 static char datadir_buffer[FN_REFLEN];
 static char mysqld_path[FN_REFLEN];
 static char *opt_datadir;
@@ -37,6 +54,7 @@ static char *opt_os_password;
 static my_bool opt_default_user;
 static my_bool opt_allow_remote_root_access;
 static my_bool opt_skip_networking;
+static my_bool verbose_errors;
 
 
 static struct my_option my_long_options[]=
@@ -45,13 +63,13 @@ static struct my_option my_long_options[]=
    NO_ARG, 0, 0, 0, 0, 0, 0},
   {"datadir", 'd', "Data directory of the new database",
   &opt_datadir, &opt_datadir, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
-  {"service", 's', "Name of the Windows service",
+  {"service", 'S', "Name of the Windows service",
   &opt_service, &opt_service, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
   {"password", 'p', "Root password",
   &opt_password, &opt_password, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
   {"port", 'P', "mysql port",
   &opt_port, &opt_port, 0, GET_INT, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
-  {"socket", 'S', 
+  {"socket", 'W', 
   "named pipe name (if missing, it will be set the same as service)",
   &opt_socket, &opt_socket, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
   {"default-user", 'D', "Create default user",
@@ -63,6 +81,8 @@ static struct my_option my_long_options[]=
   {"skip-networking", 'N', "Do not use TCP connections, use pipe instead",
   &opt_skip_networking, &opt_skip_networking, 0 , GET_BOOL, OPT_ARG, 0, 0, 0, 0,
   0, 0},
+  {"silent", 's', "Print less information", &opt_silent,
+   &opt_silent, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
   {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}
 };
 
@@ -91,13 +111,18 @@ static void die(const char *fmt, ...)
 
   /* Print the error message */
   va_start(args, fmt);
-  if (fmt)
-  {
   fprintf(stderr, "FATAL ERROR: ");
   vfprintf(stderr, fmt, args);
-    fprintf(stderr, "\n");
-    fflush(stderr);
+  fputc('\n', stderr);
+  if (verbose_errors)
+  {
+   fprintf(stderr,
+   "http://kb.askmonty.org/v/installation-issues-on-windows contains some help\n"
+   "for solving the most common problems.  If this doesn't help you, please\n"
+   "leave a comment in the knowledge base or file a bug report at\n"
+   "https://bugs.launchpad.net/maria");
   }
+  fflush(stderr);
   va_end(args);
   my_end(0);
   exit(1);
@@ -113,12 +138,9 @@ static void verbose(const char *fmt, ...)
 
   /* Print the verbose message */
   va_start(args, fmt);
-  if (fmt)
-  {
   vfprintf(stdout, fmt, args);
-    fprintf(stdout, "\n");
+  fputc('\n', stdout);
   fflush(stdout);
-  }
   va_end(args);
 }
 
@@ -126,41 +148,45 @@ static void verbose(const char *fmt, ...)
 int main(int argc, char **argv)
 {
   int error;
-  MY_INIT(argv[0]);
   char self_name[FN_REFLEN];
   char *p;
 
+  MY_INIT(argv[0]);
   GetModuleFileName(NULL, self_name, FN_REFLEN);
   strcpy(mysqld_path,self_name);
-  p = strrchr(mysqld_path, FN_LIBCHAR);
-  if(p)
+  p= strrchr(mysqld_path, FN_LIBCHAR);
+  if (p)
   {
     strcpy(p, "\\mysqld.exe");
   }
 
   if ((error= handle_options(&argc, &argv, my_long_options, get_one_option)))
     exit(error);
-  if(!opt_datadir)
+  if (!opt_datadir)
   {
     my_print_help(my_long_options);
-    die("parameter datadir is mandatory");
+    die("parameter --datadir=# is mandatory");
   }
-  if(!opt_os_user)
+
+  /* Print some help on errors */
+  verbose_errors= TRUE;
+
+  if (!opt_os_user)
   {
     opt_os_user= default_os_user;
-    opt_os_password = NULL;
+    opt_os_password= NULL;
   }
   /* Workaround WiX bug (strip possible quote character at the end of path) */
   size_t len= strlen(opt_datadir);
-  if(len > 0)
+  if (len > 0)
   {
-    if(opt_datadir[len-1] == '"')
+    if (opt_datadir[len-1] == '"')
     {
       opt_datadir[len-1]= 0;
     }
   }
   GetFullPathName(opt_datadir, FN_REFLEN, datadir_buffer, NULL);
-  opt_datadir = datadir_buffer;
+  opt_datadir= datadir_buffer;
 
   if (create_db_instance())
   {
@@ -176,39 +202,42 @@ int main(int argc, char **argv)
 /**
   Convert slashes in paths into MySQL-compatible form
 */
+
 static void convert_slashes(char *s)
 {
-  for(size_t i=0; i< strlen(s); i++)
-    if(s[i] == '\\')
-      s[i] = '/';
+  for (; *s ; s++)
+   if (*s == '\\')
+     *s= '/';
 }
 
 
-
 /**
-  Calculate basedir from mysqld.exe path
+  Calculate basedir from mysqld.exe path.
+  Basedir assumed to be is one level up from the mysqld.exe directory location.
+  E.g basedir for C:\my\bin\mysqld.exe would be C:\my
 */
+
 static void get_basedir(char *basedir, int size, const char *mysqld_path)
 {
   strcpy_s(basedir, size,  mysqld_path);
   convert_slashes(basedir);
-  char *p = strrchr(basedir,'/');
-  if(p)
+  char *p= strrchr(basedir,'/');
+  if (p)
   {
     *p = 0;
-    p=strrchr(basedir, '/');
-    if(p)
-      *p=0;
+    p= strrchr(basedir, '/');
+    if (p)
+      *p= 0;
   }
 }
 
 
-
 /**
   Allocate and initialize command line for mysqld --bootstrap.
  The resulting string is passed to popen, so it has a lot of quoting
  quoting around the full string plus quoting around parameters with spaces.
 */
+
 static char *init_bootstrap_command_line(char *cmdline, size_t size)
 {
   char basedir[MAX_PATH];
@@ -226,8 +255,9 @@ static char *init_bootstrap_command_line(char *cmdline, size_t size)
 
 /**
   Create my.ini in  current directory (this is assumed to be
-  data directory as well.
+  data directory as well).
 */
+
 static int create_myini()
 {
   my_bool enable_named_pipe= FALSE;
@@ -237,33 +267,31 @@ static int create_myini()
   GetCurrentDirectory(MAX_PATH, path_buf);
 
   /* Create ini file. */
-  FILE *myini = fopen("my.ini","wt");
-  if(!myini)
+  FILE *myini= fopen("my.ini","wt");
+  if (!myini)
   {
     die("Cannot create my.ini in data directory");
   }
-  /*
-    Write out server settings. datadir and basedir are calculated,
-    using path to mysqld.exe.
-  */
+
+  /* Write out server settings. */
   fprintf(myini, "[mysqld]\n");
   convert_slashes(path_buf);
   fprintf(myini, "datadir=%s\n", path_buf);
   if (opt_skip_networking)
   {
     fprintf(myini,"skip-networking\n");
-    if(!opt_socket)
+    if (!opt_socket)
       opt_socket= opt_service;
   }
   enable_named_pipe= (my_bool) 
     ((opt_socket && opt_socket[0]) || opt_skip_networking);
 
-  if(enable_named_pipe)
+  if (enable_named_pipe)
   {
     fprintf(myini,"enable-named-pipe\n");
   }
 
-  if(opt_socket && opt_socket[0])
+  if (opt_socket && opt_socket[0])
   {
     fprintf(myini, "socket=%s\n", opt_socket);
   }
@@ -274,11 +302,13 @@ static int create_myini()
 
   /* Write out client settings. */
   fprintf(myini, "[client]\n");
-  if(opt_socket && opt_socket[0])
+
+  /* Used for named pipes */
+  if (opt_socket && opt_socket[0])
     fprintf(myini,"socket=%s\n",opt_socket);
-  if(opt_skip_networking)
+  if (opt_skip_networking)
     fprintf(myini,"protocol=pipe\n");
-  if(opt_port)
+  else if (opt_port)
     fprintf(myini,"port=%d\n",opt_port);
   fclose(myini);
   return 0;
@@ -292,7 +322,7 @@ static const char update_root_passwd_part2[]=
 static const char remove_default_user_cmd[]= 
   "DELETE FROM mysql.user where User='';\n";
 static const char allow_remote_root_access_cmd[]=
-  "CREATE TEMPORARY TABLE tmp_user LIKE user;\n"
+  "CREATE TEMPORARY TABLE tmp_user LIKE user engine=memory;\n"
   "INSERT INTO tmp_user SELECT * from user where user='root' "
     " AND host='localhost';\n"
   "UPDATE tmp_user SET host='%';\n"
@@ -301,45 +331,42 @@ static const char allow_remote_root_access_cmd[]=
 static const char end_of_script[]="-- end.";
 
 /* Register service. Assume my.ini is in datadir */
+
 static int register_service()
 {
   char buf[3*MAX_PATH +32]; /* path to mysqld.exe, to my.ini, service name */
   SC_HANDLE sc_manager, sc_service;
 
   size_t datadir_len= strlen(opt_datadir);
-  const char *backslash_after_datadir="\\";
+  const char *backslash_after_datadir= "\\";
 
-  if (datadir_len && opt_datadir[datadir_len-1] =='\\')
-    backslash_after_datadir="";
+  if (datadir_len && opt_datadir[datadir_len-1] == '\\')
+    backslash_after_datadir= "";
 
-  verbose("Registering service");
+  verbose("Registering service '%s'", opt_service);
   my_snprintf(buf, sizeof(buf)-1,
     "\"%s\" \"--defaults-file=%s%smy.ini\" \"%s\"" ,  mysqld_path, opt_datadir, 
     backslash_after_datadir, opt_service);
 
   /* Get a handle to the SCM database. */ 
-  sc_manager= OpenSCManager( 
-    NULL,
-    NULL,
-    SC_MANAGER_ALL_ACCESS);
-
+  sc_manager= OpenSCManager( NULL, NULL, SC_MANAGER_ALL_ACCESS);
   if (!sc_manager) 
   {
-    die("OpenSCManager failed (%d)\n", GetLastError());
+    die("OpenSCManager failed (%u)\n", GetLastError());
   }
 
   /* Create the service. */
-  sc_service = CreateServiceA(sc_manager, opt_service,  opt_service,
+  sc_service= CreateService(sc_manager, opt_service,  opt_service,
     SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS, SERVICE_AUTO_START, 
     SERVICE_ERROR_NORMAL, buf, NULL, NULL, NULL, opt_os_user, opt_os_password);
 
   if (!sc_service) 
   {
     CloseServiceHandle(sc_manager);
-    die("CreateService failed (%d)", GetLastError());
+    die("CreateService failed (%u)", GetLastError());
   }
 
-  SERVICE_DESCRIPTION sd = { "MariaDB database server" };
+  SERVICE_DESCRIPTION sd= { "MariaDB database server" };
   ChangeServiceConfig2(sc_service, SERVICE_CONFIG_DESCRIPTION, &sd);
   CloseServiceHandle(sc_service); 
   CloseServiceHandle(sc_manager);
@@ -350,10 +377,7 @@ static int register_service()
 static void clean_directory(const char *dir)
 {
   char dir2[MAX_PATH+2];
-  size_t len = strlen(dir);
-
-  strcpy_s(dir2, MAX_PATH+2, dir);
-  dir2[len+1] = 0;
+  *(strmake(dir2, dir, MAX_PATH+1)+1)= 0;
 
   SHFILEOPSTRUCT fileop;
   fileop.hwnd= NULL;    /* no status display */
@@ -370,10 +394,12 @@ static void clean_directory(const char *dir)
   SHFileOperation(&fileop);
 }
 
+
 /*
   Define directory permission to have inheritable all access for a user
   (defined as username or group string or as SID)
 */
+
 static int set_directory_permissions(const char *dir, const char *os_user)
 {
 
@@ -382,103 +408,105 @@ static int set_directory_permissions(const char *dir, const char *os_user)
         BYTE buffer[SECURITY_MAX_SID_SIZE];
    } tokenInfoBuffer;
 
-  HANDLE hDir = CreateFile(dir,READ_CONTROL|WRITE_DAC,0,NULL,OPEN_EXISTING,
+  HANDLE hDir= CreateFile(dir,READ_CONTROL|WRITE_DAC,0,NULL,OPEN_EXISTING,
     FILE_FLAG_BACKUP_SEMANTICS,NULL);
-  if(hDir == INVALID_HANDLE_VALUE) 
+  if (hDir == INVALID_HANDLE_VALUE) 
     return -1;  
   ACL* pOldDACL;
-  SECURITY_DESCRIPTOR* pSD = NULL; 
+  SECURITY_DESCRIPTOR* pSD= NULL; 
   EXPLICIT_ACCESS ea={0};
   BOOL isWellKnownSID= FALSE;
   WELL_KNOWN_SID_TYPE wellKnownSidType = WinNullSid;
-  PSID pSid = NULL;
+  PSID pSid= NULL;
 
   GetSecurityInfo(hDir, SE_FILE_OBJECT , DACL_SECURITY_INFORMATION,NULL, NULL,
     &pOldDACL, NULL, (void**)&pSD); 
 
-  if(os_user)
+  if (os_user)
   {
     /* Check for 3 predefined service users 
        They might have localized names in non-English Windows, thus they need
        to be handled using well-known SIDs.
     */
-    if(stricmp(os_user, "NT AUTHORITY\\NetworkService") == 0)
+    if (stricmp(os_user, "NT AUTHORITY\\NetworkService") == 0)
     {
       wellKnownSidType= WinNetworkServiceSid;
     }
-    else if(stricmp(os_user, "NT AUTHORITY\\LocalService") == 0)
+    else if (stricmp(os_user, "NT AUTHORITY\\LocalService") == 0)
     {
       wellKnownSidType= WinLocalServiceSid;
     }
-    else if(stricmp(os_user, "NT AUTHORITY\\LocalSystem") == 0)
+    else if (stricmp(os_user, "NT AUTHORITY\\LocalSystem") == 0)
     {
       wellKnownSidType= WinLocalSystemSid;
     }
 
-    if(wellKnownSidType != WinNullSid)
+    if (wellKnownSidType != WinNullSid)
     {
-      DWORD size = SECURITY_MAX_SID_SIZE;
+      DWORD size= SECURITY_MAX_SID_SIZE;
       pSid= (PSID)tokenInfoBuffer.buffer;
       if (!CreateWellKnownSid(wellKnownSidType, NULL, pSid,
         &size))
       {
         return 1;
       }
-      ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
-      ea.Trustee.ptstrName = (LPTSTR)pSid;
+      ea.Trustee.TrusteeForm= TRUSTEE_IS_SID;
+      ea.Trustee.ptstrName= (LPTSTR)pSid;
     }
     else
     {
-      ea.Trustee.TrusteeForm = TRUSTEE_IS_NAME;
-      ea.Trustee.ptstrName = (LPSTR)os_user;
+      ea.Trustee.TrusteeForm= TRUSTEE_IS_NAME;
+      ea.Trustee.ptstrName= (LPSTR)os_user;
     }
   }
   else
   {
     HANDLE token;
-    if(OpenProcessToken(GetCurrentProcess(),TOKEN_QUERY, &token))
+    if (OpenProcessToken(GetCurrentProcess(),TOKEN_QUERY, &token))
     {
 
-      DWORD length=(DWORD) sizeof(tokenInfoBuffer);
+      DWORD length= (DWORD) sizeof(tokenInfoBuffer);
       if (GetTokenInformation(token, TokenUser, &tokenInfoBuffer, 
         length, &length))
       {
         pSid= tokenInfoBuffer.tokenUser.User.Sid;
       }
     }
-    if(!pSid)
+    if (!pSid)
       return 0;
-    ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
-    ea.Trustee.ptstrName = (LPTSTR)pSid;
+    ea.Trustee.TrusteeForm= TRUSTEE_IS_SID;
+    ea.Trustee.ptstrName= (LPTSTR)pSid;
   }
-  ea.grfAccessMode = GRANT_ACCESS;
-  ea.grfAccessPermissions = GENERIC_ALL; 
-  ea.grfInheritance = CONTAINER_INHERIT_ACE|OBJECT_INHERIT_ACE; 
-  ea.Trustee.TrusteeType = TRUSTEE_IS_UNKNOWN; 
-  ACL* pNewDACL = 0; 
-  DWORD err = SetEntriesInAcl(1,&ea,pOldDACL,&pNewDACL); 
-  if(pNewDACL)
+  ea.grfAccessMode= GRANT_ACCESS;
+  ea.grfAccessPermissions= GENERIC_ALL; 
+  ea.grfInheritance= CONTAINER_INHERIT_ACE|OBJECT_INHERIT_ACE; 
+  ea.Trustee.TrusteeType= TRUSTEE_IS_UNKNOWN; 
+  ACL* pNewDACL= 0; 
+  DWORD err= SetEntriesInAcl(1,&ea,pOldDACL,&pNewDACL); 
+  if (pNewDACL)
   {
     SetSecurityInfo(hDir,SE_FILE_OBJECT,DACL_SECURITY_INFORMATION,NULL, NULL,
       pNewDACL, NULL);
   }
-  if(pSD != NULL) 
+  if (pSD != NULL) 
     LocalFree((HLOCAL) pSD); 
-  if(pNewDACL != NULL) 
+  if (pNewDACL != NULL) 
     LocalFree((HLOCAL) pNewDACL);
   CloseHandle(hDir); 
   return 0;
 }
 
+
 /* 
   Give directory permissions for special service user NT SERVICE\servicename
   this user is available only on Win7 and later.
 */
+
 void grant_directory_permissions_to_service()
 {
   char service_user[MAX_PATH+ 12];
   OSVERSIONINFO info;
-  info.dwOSVersionInfoSize = sizeof(info);
+  info.dwOSVersionInfoSize= sizeof(info);
   GetVersionEx(&info);
   if (info.dwMajorVersion >6 || 
     (info.dwMajorVersion== 6 && info.dwMinorVersion > 0)
@@ -490,10 +518,12 @@ void grant_directory_permissions_to_service()
   }
 }
 
+
 /* Create database instance (including registering as service etc) .*/
+
 static int create_db_instance()
 {
-  int ret=0;
+  int ret= 0;
   char cwd[MAX_PATH];
   DWORD cwd_len= MAX_PATH;
   char cmdline[3*MAX_PATH];
@@ -504,30 +534,34 @@ static int create_db_instance()
   GetCurrentDirectory(cwd_len, cwd);
   CreateDirectory(opt_datadir, NULL); /*ignore error, it might already exist */
 
-  if(!SetCurrentDirectory(opt_datadir))
+  if (!SetCurrentDirectory(opt_datadir))
   {
-    die("Cannot set current directory to %s\n",opt_datadir);
+    die("Cannot set current directory to '%s'\n",opt_datadir);
     return -1;
   }
 
   CreateDirectory("mysql",NULL);
   CreateDirectory("test", NULL);
 
+  /*
+    Set data directory permissions for both current user and 
+    default_os_user (the one who runs services).
+  */
   set_directory_permissions(opt_datadir, NULL);
   set_directory_permissions(opt_datadir, default_os_user);
 
-  /* Create mysqld --bootstrap process */
+  /* Do mysqld --bootstrap. */
   init_bootstrap_command_line(cmdline, sizeof(cmdline));
   /* verbose("Executing %s", cmdline); */
 
   in= popen(cmdline, "wt");
-  if(!in)
+  if (!in)
     goto end;
 
   if (fwrite("use mysql;\n",11,1, in) != 1)
   {
     verbose("ERROR: Cannot write to mysqld's stdin");
-    ret = 1;
+    ret= 1;
     goto end;
   }
 
@@ -539,16 +573,15 @@ static int create_db_instance()
     goto end;
   }
 
-  
   /* Remove default user, if requested. */
-  if(!opt_default_user)
+  if (!opt_default_user)
   {
     verbose("Removing default user",remove_default_user_cmd);
     fputs(remove_default_user_cmd, in);
     fflush(in);
   }
 
-  if(opt_allow_remote_root_access)
+  if (opt_allow_remote_root_access)
   {
      verbose("Allowing remote access for user root",remove_default_user_cmd);
      fputs(allow_remote_root_access_cmd,in);
@@ -565,7 +598,6 @@ static int create_db_instance()
     fflush(in);
   }
 
-
   /*
     On some reason, bootstrap chokes if last command sent via stdin ends with 
     newline, so we supply a dummy comment, that does not end with newline.
@@ -577,21 +609,21 @@ static int create_db_instance()
   ret= pclose(in);
   if (ret)
   {
-    verbose("mysqld returned an error in pclose");
+    verbose("mysqld returned error %d in pclose",ret);
     goto end;
   }
 
   /* Create my.ini file in data directory.*/
   ret= create_myini();
-  if(ret)
+  if (ret)
     goto end;
 
   /* Register service if requested. */
-  if(opt_service && opt_service[0])
+  if (opt_service && opt_service[0])
   {
     ret= register_service();
     grant_directory_permissions_to_service();
-    if(ret)
+    if (ret)
       goto end;
   }
 

sql/mysql_upgrade_service.cc

-/* Copyright (C) 2010 Monty Program Ab
+/* Copyright (C) 2010-2011 Monty Program Ab & Vladislav Vaintroub
 
    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
@@ -28,15 +28,14 @@
 #include <mysql_version.h>
 #include <winservice.h>
 
-#include <stdlib.h>
-#include <stdio.h>
 #include <windows.h>
 
 /* We're using version APIs */
 #pragma comment(lib, "version")
 
 #define USAGETEXT \
-"mysql_upgrade_service.exe  Ver 1.42 for Windows\n" \
+"mysql_upgrade_service.exe  Ver 1.00 for Windows\n" \
+"Copyright (C) 2010-2011 Monty Program Ab & Vladislav Vaintroub" \
 "This software comes with ABSOLUTELY NO WARRANTY. This is free software,\n" \
 "and you are welcome to modify and redistribute it under the GPL v2 license\n" \
 "Usage: mysql_upgrade_service.exe [OPTIONS]\n" \
@@ -66,7 +65,7 @@ static struct my_option my_long_options[]=
 {
   {"help", '?', "Display this help message and exit.", 0, 0, 0, GET_NO_ARG,
    NO_ARG, 0, 0, 0, 0, 0, 0},
-  {"service", 's', "Name of the existing Windows service",
+  {"service", 'S', "Name of the existing Windows service",
   &opt_service, &opt_service, 0, GET_STR, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
 };
 
@@ -93,17 +92,12 @@ get_one_option(int optid,
 static void log(const char *fmt, ...)
 {
   va_list args;
-  char buf[4096];
-
   /* Print the error message */
   va_start(args, fmt);
-  if (fmt)
-  {
-    vsprintf_s(buf,  fmt, args);
-    fprintf(stdout, "%s\n", buf);
-  }
+  vfprintf(stdout,fmt, args);
   va_end(args);
-  my_end(0);
+  fputc('\n', stdout);
+  fflush(stdout);
 }
 
 
@@ -111,46 +105,47 @@ static void die(const char *fmt, ...)
 {
   va_list args;
   DBUG_ENTER("die");
-  char buf[4096];
 
   /* Print the error message */
   va_start(args, fmt);
-  if (fmt)
-  {
+
   fprintf(stderr, "FATAL ERROR: ");
-    int count= vsprintf_s(buf,  fmt, args);
-    fprintf(stderr, "%s.", buf);
-    if(logfile_path[0])
+  vfprintf(stderr, fmt, args);
+  if (logfile_path[0])
   {
     fprintf(stderr, "Additional information can be found in the log file %s",
       logfile_path);
   }
-  }
   va_end(args);
-
+  fputc('\n', stderr);
+  fflush(stdout);
   /* Cleanup */
-  if(service && initial_service_state != -1)
-  {
-    /* Stop service if it was not running */
-    if(initial_service_state != SERVICE_RUNNING)
+
+  /*
+    Stop service that we started, if it was not initally running at
+    program start.
+  */
+  if (initial_service_state != -1 && initial_service_state != SERVICE_RUNNING)
   {
     SERVICE_STATUS service_status;
     ControlService(service, SERVICE_CONTROL_STOP, &service_status);
   }
-    CloseServiceHandle(service);
-  }
-  if(scm)
-    CloseServiceHandle(scm);
 
-  /* Stop mysqld.exe if it was started for upgrade */
-  if(mysqld_process)
+  if (scm)
+    CloseServiceHandle(scm);
+  if (service)
+    CloseServiceHandle(service);
+  /* Stop mysqld.exe, if it was started for upgrade */
+  if (mysqld_process)
     TerminateProcess(mysqld_process, 3);
-  if(logfile_handle)
+  if (logfile_handle)
     CloseHandle(logfile_handle);
   my_end(0);
+
   exit(1);
 }
 
+
 /*
   spawn-like function to run subprocesses. 
   We also redirect the full output to the log file.
@@ -166,43 +161,44 @@ static void die(const char *fmt, ...)
   @return intptr containing either process handle, if P_NOWAIT is used
   or return code of the process (if P_WAIT is used)
 */
+
 static intptr_t run_tool(int wait_flag, const char *program,...)
 {
   static char cmdline[32*1024];
+  char *end;
   va_list args;
   va_start(args, program);
-  if(!program)
+  if (!program)
     die("Invalid call to run_tool");
+  end= strxmov(cmdline, "\"", program, "\"", NullS);
 
-  strcpy_s(cmdline, "\"");
-  strcat_s(cmdline, program);
-  strcat_s(cmdline, "\"");
   for(;;) 
   {
     char *param= va_arg(args,char *);
     if(!param)
       break;
-    strcat_s(cmdline, " \"");
-    strcat_s(cmdline, param);
-    strcat_s(cmdline, "\"");
+    end= strxmov(end, " \"", param, "\"", NullS);
   }
   va_end(args);
   
   /* Create output file if not alredy done */
-  if(!logfile_handle)
+  if (!logfile_handle)
   {
     char tmpdir[FN_REFLEN];
     GetTempPath(FN_REFLEN, tmpdir);
     sprintf_s(logfile_path, "%s\\mysql_upgrade_service.%s.log", tmpdir, 
       opt_service);
-    logfile_handle = CreateFile(logfile_path, GENERIC_WRITE,  FILE_SHARE_READ, 
+    logfile_handle= CreateFile(logfile_path, GENERIC_WRITE,  FILE_SHARE_READ, 
       NULL, TRUNCATE_EXISTING, 0, NULL);
-    if(!logfile_handle)
-      die("Cannot open log file %s", logfile_path);
+    if (!logfile_handle)
+    {
+      die("Cannot open log file %s, windows error %u", 
+        logfile_path, GetLastError());
+    }
   }
 
   /* Start child process */
-  STARTUPINFO si={0};
+  STARTUPINFO si= {0};
   si.cb= sizeof(si);
   si.hStdInput= GetStdHandle(STD_INPUT_HANDLE);
   si.hStdError= logfile_handle;
@@ -216,13 +212,13 @@ static intptr_t run_tool(int wait_flag, const char *program,...)
   }
   CloseHandle(pi.hThread);
 
-  if(wait_flag == P_NOWAIT)
+  if (wait_flag == P_NOWAIT)
   {
-    /* Do not wait for process to complete, return handle */
+    /* Do not wait for process to complete, return handle. */
     return (intptr_t)pi.hProcess;
   }
 
-  /* Eait for process to complete */
+  /* Wait for process to complete. */
   if (WaitForSingleObject(pi.hProcess, INFINITE) != WAIT_OBJECT_0)
   {
     die("WaitForSingleObject() failed");
@@ -236,7 +232,6 @@ static intptr_t run_tool(int wait_flag, const char *program,...)
 }
 
 
-
 void stop_mysqld_service()
 {
   DWORD needed;
@@ -249,7 +244,7 @@ void stop_mysqld_service()
           sizeof(SERVICE_STATUS_PROCESS),
           &needed))
     {
-      die("QueryServiceStatusEx failed (%d)\n", GetLastError()); 
+      die("QueryServiceStatusEx failed (%u)\n", GetLastError()); 
     }
 
     /*
@@ -257,7 +252,7 @@ void stop_mysqld_service()
       exit.
     */
     if(initial_service_state == -1)
-      initial_service_state =ssp.dwCurrentState;
+      initial_service_state= ssp.dwCurrentState;
 
     switch(ssp.dwCurrentState)
     {
@@ -266,12 +261,13 @@ void stop_mysqld_service()
       case SERVICE_RUNNING:
         if(!ControlService(service, SERVICE_CONTROL_STOP, 
              (SERVICE_STATUS *)&ssp))
-            die("ControlService failed, error %d\n", GetLastError());
+            die("ControlService failed, error %u\n", GetLastError());
       case SERVICE_START_PENDING:
       case SERVICE_STOP_PENDING:
         if(timeout < 0)
-          die("Service does not stop after 1 minute timeout");
+          die("Service does not stop after %d seconds timeout",shutdown_timeout);
         Sleep(100);
+        timeout -= 100;
         break;
       default:
         die("Unexpected service state %d",ssp.dwCurrentState);
@@ -280,33 +276,6 @@ void stop_mysqld_service()
 }
 
 
-/* Helper routine. Used to prevent downgrades by mysql_upgrade_service */
-void get_file_version(const wchar_t *path, int *major, int *minor)
-{
-  *major= *minor=0;
-  DWORD version_handle;
-  char *ver= 0;
-  VS_FIXEDFILEINFO info;
-  UINT len;
-  void *p;
-
-  DWORD size = GetFileVersionInfoSizeW(path, &version_handle);
-  if (size == 0) 
-    return;
-  ver = new char[size];
-  if(!GetFileVersionInfoW(path, version_handle, size, ver))
-    goto end;
-
-  if(!VerQueryValue(ver,"\\",&p,&len))
-    goto end;
-  memcpy(&info,p ,sizeof(VS_FIXEDFILEINFO));
-
-  *major = (info.dwFileVersionMS & 0xFFFF0000) >> 16;
-  *minor = (info.dwFileVersionMS & 0x0000FFFF);
-end:
-  delete []ver;
-}
-
 /* 
   Shutdown mysql server. Not using mysqladmin, since 
   our --skip-grant-tables do not work anymore after mysql_upgrade
@@ -340,21 +309,22 @@ static void change_service_config()
   char defaults_file[MAX_PATH];
   char default_character_set[64];
   char buf[MAX_PATH];
+  char commandline[3*MAX_PATH + 19];
   int i;
 
-  scm = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
+  scm= OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
   if(!scm)
-    die("OpenSCManager failed with %d", GetLastError());
+    die("OpenSCManager failed with %u", GetLastError());
   service= OpenService(scm, opt_service, SERVICE_ALL_ACCESS);
   if (!service)
-    die("OpenService failed with %d", GetLastError());
+    die("OpenService failed with %u", GetLastError());
 
   BYTE config_buffer[8*1024];
   LPQUERY_SERVICE_CONFIGW config= (LPQUERY_SERVICE_CONFIGW)config_buffer;
-  DWORD size=sizeof(config_buffer);
+  DWORD size= sizeof(config_buffer);
   DWORD needed;
   if (!QueryServiceConfigW(service, config, size, &needed))
-    die("QueryServiceConfig failed with %d\n", GetLastError());
+    die("QueryServiceConfig failed with %u", GetLastError());
 
   mysqld_service_properties props;
   if (get_mysql_service_properties(config->lpBinaryPathName, &props))
@@ -363,14 +333,15 @@ static void change_service_config()
   }
 
   int my_major= MYSQL_VERSION_ID/10000;
-  int my_minor= (MYSQL_VERSION_ID - 10000*my_major)/100;
+  int my_minor= (MYSQL_VERSION_ID %10000)/100;
+  int my_patch= MYSQL_VERSION_ID%100;
 
   if(my_major < props.version_major || 
-    (my_major == props.version_minor && my_minor < props.version_patch))
+    (my_major == props.version_major && my_minor < props.version_minor))
   {
-    die("Can not downgrade, the service is currently running as version %d.%d"
-      ", my version is %d.%d", props.version_major, props.version_minor, 
-      my_major, my_minor);
+    die("Can not downgrade, the service is currently running as version %d.%d.%d"
+      ", my version is %d.%d.%d", props.version_major, props.version_minor, 
+      props.version_patch, my_major, my_minor, my_patch);
   }
 
   if(props.inifile[0] == 0)
@@ -386,7 +357,7 @@ static void change_service_config()
     unix style slashes.
   */
   strcpy_s(buf, MAX_PATH, props.datadir);
-  for(i=0; buf[i]; i++)
+  for(i= 0; buf[i]; i++)
   {
     if (buf[i] == '\\')
       buf[i]= '/';
@@ -404,10 +375,10 @@ static void change_service_config()
     "default-character-set is deprecated and will be replaced ..."
     message.
   */
-  default_character_set[0]=0;
+  default_character_set[0]= 0;
   GetPrivateProfileString("mysqld", "default-character-set", NULL,
     default_character_set, sizeof(default_character_set), defaults_file);
-  if(default_character_set[0])
+  if (default_character_set[0])
   {
     WritePrivateProfileString("mysqld", "default-character-set", NULL, 
       defaults_file);
@@ -416,19 +387,17 @@ static void change_service_config()
   }
 
   sprintf(defaults_file_param,"--defaults-file=%s", props.inifile);
-  char commandline[3*MAX_PATH + 19];
   sprintf_s(commandline, "\"%s\" \"%s\" \"%s\"", mysqld_path, 
    defaults_file_param, opt_service);
   if (!ChangeServiceConfig(service, SERVICE_NO_CHANGE, SERVICE_NO_CHANGE, 
          SERVICE_NO_CHANGE, commandline, NULL, NULL, NULL, NULL, NULL, NULL))
   {
-    die("ChangeServiceConfigW failed with %d", GetLastError());
+    die("ChangeServiceConfig failed with %u", GetLastError());
   }
 
 }
 
 
-
 int main(int argc, char **argv)
 {
   int error;
@@ -439,8 +408,8 @@ int main(int argc, char **argv)
   /* Parse options */
   if ((error= handle_options(&argc, &argv, my_long_options, get_one_option)))
     die("");
-  if(!opt_service)
-    die("service parameter is mandatory");
+  if (!opt_service)
+    die("--service=# parameter is mandatory");
  
  /*
     Get full path to mysqld, we need it when changing service configuration.
@@ -448,17 +417,17 @@ int main(int argc, char **argv)
     and mysql_upgrade_service.exe are in the same directory.
   */
   GetModuleFileName(NULL, bindir, FN_REFLEN);
-  p = strrchr(bindir, FN_LIBCHAR);
+  p= strrchr(bindir, FN_LIBCHAR);
   if(p)
   {
-    *p=0;
+    *p= 0;
   }
   sprintf_s(mysqld_path, "%s\\mysqld.exe", bindir);
   sprintf_s(mysqladmin_path, "%s\\mysqladmin.exe", bindir);
   sprintf_s(mysqlupgrade_path, "%s\\mysql_upgrade.exe", bindir);
 
   char *paths[]= {mysqld_path, mysqladmin_path, mysqlupgrade_path};
-  for(int i=0; i< 3;i++)
+  for(int i= 0; i< 3;i++)
   {
     if(GetFileAttributes(paths[i]) == INVALID_FILE_ATTRIBUTES)
       die("File %s does not exist", paths[i]);
@@ -466,7 +435,7 @@ int main(int argc, char **argv)
 
   /*
     Messages written on stdout should not be buffered,  GUI upgrade program 
-    read them from pipe and uses as progress indicator.
+    reads them from pipe and uses as progress indicator.
   */
   setvbuf(stdout, NULL, _IONBF, 0);
 
@@ -476,7 +445,6 @@ int main(int argc, char **argv)
   log("Phase 2/8: Stopping service");
   stop_mysqld_service();
 
-
   /* 
     Start mysqld.exe as non-service skipping privileges (so we do not 
     care about the password). But disable networking and enable pipe 
@@ -491,14 +459,14 @@ int main(int argc, char **argv)
     defaults_file_param, "--skip-networking",  "--skip-grant-tables", 
     "--enable-named-pipe",  socket_param, NULL);
 
-  if(mysqld_process == INVALID_HANDLE_VALUE)
+  if (mysqld_process == INVALID_HANDLE_VALUE)
   {
     die("Cannot start mysqld.exe process, errno=%d", errno);
   }
 
   log("Phase 4/8: Waiting for startup to complete");
   DWORD start_duration_ms= 0;
-  for (int i=0; ; i++)
+  for(;;)
   {
     if (WaitForSingleObject(mysqld_process, 0) != WAIT_TIMEOUT)
       die("mysqld.exe did not start");
@@ -515,10 +483,13 @@ int main(int argc, char **argv)
   }
 
   log("Phase 5/8: Running mysql_upgrade");
-  int upgrade_err = (int) run_tool(P_WAIT,  mysqlupgrade_path, 
+  int upgrade_err= (int) run_tool(P_WAIT,  mysqlupgrade_path, 
     "--protocol=pipe", "--force",  socket_param,
     NULL);
 
+  if (upgrade_err)
+    die("mysql_upgrade failed with error code %d\n", upgrade_err);
+
   log("Phase 6/8: Initiating server shutdown");
   initiate_mysqld_shutdown();
 
@@ -543,7 +514,7 @@ int main(int argc, char **argv)
     opt_service, logfile_path);
   CloseServiceHandle(service);
   CloseServiceHandle(scm);
-  if(logfile_handle)
+  if (logfile_handle)
     CloseHandle(logfile_handle);
   my_end(0);
   exit(0);