[PATCH 1/2] testbot/testagentd: Fix upgrades when there is more than one server running.

[Francois Gouget]

On Windows the second server will keep the old executable busy,
preventing its deletion. So give up after a while.
Usually the second server will be running with --set-time-only so
upgrading it is not as important.
If necessary this condition can be detected on the client by
attempting to delete the old executable and checking for an error.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---

Note: Patch 2 only depends on this one because this fix is mentionned in 
      the comment describing version 1.8.

 testbot/src/testagentd/platform_windows.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/testbot/src/testagentd/platform_windows.c b/testbot/src/testagentd/platform_windows.c
index bb52a0241..0ba7d2cdf 100644
--- a/testbot/src/testagentd/platform_windows.c
+++ b/testbot/src/testagentd/platform_windows.c
@@ -532,17 +532,20 @@ int platform_init(void)
     {
         /* This also serves to ensure the old server has released the port
          * before we attempt to open our own.
+         * But if a second server is running the deletion will never work so
+         * give up after a while.
          */
+        int attempt = 0;
         do
         {
             if (!DeleteFileA(oldtestagentd))
                 Sleep(500);
+            attempt++;
         }
-        while (GetLastError() ==  ERROR_ACCESS_DENIED);
+        while (GetLastError() ==  ERROR_ACCESS_DENIED && attempt < 20);
         free(oldtestagentd);
     }
 
-
     wVersionRequested = MAKEWORD(2, 2);
     rc = WSAStartup(wVersionRequested, &wsaData);
     if (rc)
-- 
2.20.1

[PATCH 2/2] testbot/TestAgent: Add the restart RPC.

[Francois Gouget]

This is similar to the upgrade RPC but leaves out sending the new server
binary and allows changing the server command line which makes it more
flexible.

Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com>
---
 testbot/lib/WineTestBot/TestAgent.pm      |  52 ++++++
 testbot/scripts/TestAgent                 |  14 +-
 testbot/src/testagentd/platform.h         |   2 +-
 testbot/src/testagentd/platform_unix.c    |  45 +++--
 testbot/src/testagentd/platform_windows.c | 199 ++++++++++++++++++++--
 testbot/src/testagentd/testagentd.c       |  72 +++++++-
 6 files changed, 356 insertions(+), 28 deletions(-)

diff --git a/testbot/lib/WineTestBot/TestAgent.pm b/testbot/lib/WineTestBot/TestAgent.pm
index 3cd72b503..a4d32ec27 100644
--- a/testbot/lib/WineTestBot/TestAgent.pm
+++ b/testbot/lib/WineTestBot/TestAgent.pm
@@ -40,6 +40,7 @@ my $RPC_UPGRADE = 9;
 my $RPC_RMCHILDPROC = 10;
 my $RPC_GETCWD = 11;
 my $RPC_SETPROPERTY = 12;
+my $RPC_RESTART = 13;
 
 my %RpcNames=(
     $RPC_PING => 'ping',
@@ -55,6 +56,7 @@ my %RpcNames=(
     $RPC_RMCHILDPROC => 'rmchildproc',
     $RPC_GETCWD => 'getcwd',
     $RPC_SETPROPERTY => 'setproperty',
+    $RPC_RESTART => 'restart',
 );
 
 my $Debug = 0;
@@ -1482,6 +1484,56 @@ sub Upgrade($$)
   return $rc;
 }
 
+sub Restart($$)
+{
+  my ($self, $Argv) = @_;
+
+  if (!$Argv || !@$Argv)
+  {
+    # Restart the server with the same parameters
+    my $Properties = $self->GetProperties();
+    my $Argc = $Properties->{"server.argc"};
+    if (!$Argc)
+    {
+      $self->_SetError($ERROR, "Could not get the server command line argument count");
+      return undef;
+    }
+    for (my $i = 0; $i < $Argc; $i++)
+    {
+      my $Arg = $Properties->{sprintf("server.argv[%d]", $i)};
+      if (!defined $Arg)
+      {
+        $self->_SetError($ERROR, "Server argument $i is undefined!");
+        return undef;
+      }
+      push @$Argv, $Arg;
+    }
+  }
+  debug("Restart TestAgentd: '", join("' '", @$Argv), "'\n");
+
+  if (!$self->_StartRPC($RPC_RESTART) or
+      !$self->_SendListSize('ArgC', scalar(@$Argv)))
+  {
+    return undef;
+  }
+  my $i = 0;
+  foreach my $Arg (@$Argv)
+  {
+    return undef if (!$self->_SendString("Cmd$i", $Arg));
+    $i++;
+  }
+
+  # Get the reply
+  my $rc = $self->_RecvList('');
+
+  # The server has quit and thus the connection is no longer usable.
+  # So disconnect now to force the next RPC to reconnect, instead or letting it
+  # try to reuse the broken connection and fail.
+  $self->Disconnect();
+
+  return $rc;
+}
+
 sub RemoveChildProcess($$)
 {
   my ($self, $Pid) = @_;
diff --git a/testbot/scripts/TestAgent b/testbot/scripts/TestAgent
index b4ec582be..84bbf7e0c 100755
--- a/testbot/scripts/TestAgent
+++ b/testbot/scripts/TestAgent
@@ -53,7 +53,7 @@ sub error(@)
 }
 
 my ($Cmd, $Hostname, $LocalFilename, $ServerFilename, $PropName, $PropValue, @Rm);
-my (@Run, $RunIn, $RunOut, $RunErr, $WaitPid);
+my (@Run, $RunIn, $RunOut, $RunErr, $WaitPid, @Restart);
 my $SendFlags = 0;
 my $RunFlags = 0;
 my ($Port, $ConnectOneTimeout, $ConnectMinAttempts, $ConnectMinTimeout, $Timeout);
@@ -240,6 +240,12 @@ while (@ARGV)
         set_cmd($arg);
         $LocalFilename = check_opt_val($arg, $LocalFilename);
     }
+    elsif ($arg eq "restart")
+    {
+        set_cmd($arg);
+        @Restart = @ARGV;
+        last;
+    }
     else
     {
         error("unknown command '$arg'\n");
@@ -359,6 +365,8 @@ if (defined $Usage)
     print "  ping          Makes sure the server is still alive.\n";
     print "  upgrade       Replaces the server executable with the specified file and\n";
     print "                restarts it.\n";
+    print "  restart       Replaces and restarts the server from the specified server file\n";
+    print "                and arguments or from the last command line if omitted.\n";
     print "  <hostname>    Is the hostname of the server.\n";
     print "  --port <port> Use the specified port number instead of the default one.\n";
     print "  --connect-one-timeout <time> Specifies the timeout for one connection\n";
@@ -477,6 +485,10 @@ elsif ($Cmd eq "upgrade")
 {
     $Result = $TA->Upgrade($LocalFilename);
 }
+elsif ($Cmd eq "restart")
+{
+    $Result = $TA->Restart(@Restart);
+}
 elsif ($Cmd eq "getcwd")
 {
     $Result = $TA->GetCwd();
diff --git a/testbot/src/testagentd/platform.h b/testbot/src/testagentd/platform.h
index 06882dedc..eb67c7afb 100644
--- a/testbot/src/testagentd/platform.h
+++ b/testbot/src/testagentd/platform.h
@@ -100,7 +100,7 @@ int platform_settime(uint64_t epoch, uint32_t leeway);
  * Returns non-zero if successful, which will let the caller know to complete
  * the current RPC and cleanly exit. Returns 0 otherwise.
  */
-int platform_upgrade(const char* tmpserver, char** argv);
+int platform_upgrade(char* current_argv0, int argc, char** argv);
 
 /* Called when the message has been dismissed by the user.
  */
diff --git a/testbot/src/testagentd/platform_unix.c b/testbot/src/testagentd/platform_unix.c
index 560d1210e..a64bd8edb 100644
--- a/testbot/src/testagentd/platform_unix.c
+++ b/testbot/src/testagentd/platform_unix.c
@@ -240,28 +240,48 @@ int platform_settime(uint64_t epoch, uint32_t leeway)
 }
 
 
-int platform_upgrade(const char* tmpserver, char** argv)
+int platform_upgrade(char* current_argv0, int argc, char** argv)
 {
-    static const char* oldserver = "testagentd.old";
+    static const char* OLDSERVER = "testagentd.old";
+    const char* oldserver = NULL;
+    struct stat stat_current, stat_argv0;
     int pipefds[2];
     pid_t pid;
 
-    if (rename(argv[0], oldserver))
+    if (stat(current_argv0, &stat_current))
     {
-        set_status(ST_ERROR, "unable to move the current server file out of the way: %s", strerror(errno));
+        set_status(ST_ERROR, "could not stat '%s': %s", current_argv0, strerror(errno));
         return 0;
     }
 
-    if (rename(tmpserver, argv[0]))
+    if (stat(argv[0], &stat_argv0))
     {
-        set_status(ST_ERROR, "unable to move the currentnew server file into place: %s", strerror(errno));
-        rename(oldserver, argv[0]);
+        set_status(ST_ERROR, "could not stat '%s': %s", argv[0], strerror(errno));
         return 0;
     }
+
+    if (stat_current.st_dev != stat_argv0.st_dev ||
+        stat_current.st_ino != stat_argv0.st_ino)
+    {
+        oldserver = OLDSERVER;
+        if (rename(current_argv0, oldserver))
+        {
+            set_status(ST_ERROR, "unable to move the current server file out of the way: %s", strerror(errno));
+            return 0;
+        }
+
+        if (rename(argv[0], current_argv0))
+        {
+            set_status(ST_ERROR, "unable to move the currentnew server file into place: %s", strerror(errno));
+            rename(oldserver, argv[0]);
+            return 0;
+        }
+    }
     if (pipe(pipefds))
     {
         set_status(ST_ERROR, "could not synchronize with the new process: %s", strerror(errno));
-        rename(oldserver, argv[0]);
+        if (oldserver)
+            rename(oldserver, current_argv0);
         return 0;
     }
 
@@ -271,11 +291,13 @@ int platform_upgrade(const char* tmpserver, char** argv)
         set_status(ST_ERROR, "unable to start the new server: %s", strerror(errno));
         close(pipefds[0]);
         close(pipefds[1]);
-        rename(oldserver, argv[0]);
+        if (oldserver)
+            rename(oldserver, current_argv0);
         return 0;
     }
 
-    unlink(oldserver);
+    if (oldserver)
+        unlink(oldserver);
     if (!pid)
     {
         /* The child process is responsible for cleanly closing the connection
@@ -292,7 +314,8 @@ int platform_upgrade(const char* tmpserver, char** argv)
     read(pipefds[0], &pid, 1);
     close(pipefds[0]);
 
-    execvp(argv[0], argv);
+    argv[0] = current_argv0;
+    execvp(current_argv0, argv);
     return 1;
 }
 
diff --git a/testbot/src/testagentd/platform_windows.c b/testbot/src/testagentd/platform_windows.c
index 0ba7d2cdf..d6f9ba96d 100644
--- a/testbot/src/testagentd/platform_windows.c
+++ b/testbot/src/testagentd/platform_windows.c
@@ -298,40 +298,212 @@ char* get_server_filename(int old)
     return strdup(filename);
 }
 
-int platform_upgrade(const char* tmpserver, char** argv)
+/***********************************************************************
+ *           build_command_line
+ *
+ * Adapted from the Wine source.
+ *
+ * Build the command line of a process from the argv array.
+ *
+ * Note that it does NOT necessarily include the file name.
+ * Sometimes we don't even have any command line options at all.
+ *
+ * We must quote and escape characters so that the argv array can be rebuilt
+ * from the command line:
+ * - spaces and tabs must be quoted
+ *   'a b'   -> '"a b"'
+ * - quotes must be escaped
+ *   '"'     -> '\"'
+ * - if '\'s are followed by a '"', they must be doubled and followed by '\"',
+ *   resulting in an odd number of '\' followed by a '"'
+ *   '\"'    -> '\\\"'
+ *   '\\"'   -> '\\\\\"'
+ * - '\'s are followed by the closing '"' must be doubled,
+ *   resulting in an even number of '\' followed by a '"'
+ *   ' \'    -> '" \\"'
+ *   ' \\'    -> '" \\\\"'
+ * - '\'s that are not followed by a '"' can be left as is
+ *   'a\b'   == 'a\b'
+ *   'a\\b'  == 'a\\b'
+ */
+static char *build_command_line(char **argv)
+{
+    int len;
+    char **arg;
+    char *p;
+    char* cmdline;
+
+    len = 0;
+    for (arg = argv; *arg; arg++)
+    {
+        BOOL has_space;
+        int bcount;
+        char* a;
+
+        has_space=FALSE;
+        bcount=0;
+        a=*arg;
+        if( !*a ) has_space=TRUE;
+        while (*a!='\0') {
+            if (*a=='\\') {
+                bcount++;
+            } else {
+                if (*a==' ' || *a=='\t') {
+                    has_space=TRUE;
+                } else if (*a=='"') {
+                    /* doubling of '\' preceding a '"',
+                     * plus escaping of said '"'
+                     */
+                    len+=2*bcount+1;
+                }
+                bcount=0;
+            }
+            a++;
+        }
+        len+=(a-*arg)+1 /* for the separating space */;
+        if (has_space)
+            len+=2+bcount; /* for the quotes and doubling of '\' preceding the closing quote */
+    }
+
+    cmdline = malloc(len);
+    if (!cmdline)
+        return NULL;
+
+    p = cmdline;
+    for (arg = argv; *arg; arg++)
+    {
+        BOOL has_space,has_quote;
+        char* a;
+        int bcount;
+
+        /* Check for quotes and spaces in this argument */
+        has_space=has_quote=FALSE;
+        a=*arg;
+        if( !*a ) has_space=TRUE;
+        while (*a!='\0') {
+            if (*a==' ' || *a=='\t') {
+                has_space=TRUE;
+                if (has_quote)
+                    break;
+            } else if (*a=='"') {
+                has_quote=TRUE;
+                if (has_space)
+                    break;
+            }
+            a++;
+        }
+
+        /* Now transfer it to the command line */
+        if (has_space)
+            *p++='"';
+        if (has_quote || has_space) {
+            bcount=0;
+            a=*arg;
+            while (*a!='\0') {
+                if (*a=='\\') {
+                    *p++=*a;
+                    bcount++;
+                } else {
+                    if (*a=='"') {
+                        int i;
+
+                        /* Double all the '\\' preceding this '"', plus one */
+                        for (i=0;i<=bcount;i++)
+                            *p++='\\';
+                        *p++='"';
+                    } else {
+                        *p++=*a;
+                    }
+                    bcount=0;
+                }
+                a++;
+            }
+        } else {
+            char* x = *arg;
+            while ((*p=*x++)) p++;
+        }
+        if (has_space) {
+            int i;
+
+            /* Double all the '\' preceding the closing quote */
+            for (i=0;i<bcount;i++)
+                *p++='\\';
+            *p++='"';
+        }
+        *p++=' ';
+    }
+    if (p > cmdline)
+        p--;  /* remove last space */
+    *p = '\0';
+
+    return cmdline;
+}
+
+int platform_upgrade(char* current_argv0, int argc, char** argv)
 {
-    char *testagentd = NULL, *oldtestagentd = NULL;
+    char *testagentd = NULL, *oldtestagentd = NULL, *cmdline = NULL;
+    char full_argv0[MAX_PATH];
+    char *tmp_argv0;
+    DWORD rc;
     STARTUPINFO si;
     PROCESS_INFORMATION pi;
     int success = 0;
 
     testagentd = get_server_filename(0);
-    oldtestagentd = get_server_filename(1);
-    if (!testagentd || !oldtestagentd)
+    if (!testagentd)
     {
-        set_status(ST_ERROR, "unable to get the process filenames (le=%lu)", GetLastError());
+        set_status(ST_ERROR, "unable to get the process filename (le=%lu)", GetLastError());
         goto done;
     }
 
-    if (!MoveFile(testagentd, oldtestagentd))
+    rc = GetFullPathNameA(argv[0], sizeof(full_argv0), full_argv0, NULL);
+    if (rc == 0 || rc > sizeof(full_argv0))
     {
-        set_status(ST_ERROR, "unable to move the current server file out of the way (le=%lu)", GetLastError());
+        set_status(ST_ERROR, "could not get the full path for '%s' (le=%lu)", argv[0], GetLastError());
         goto done;
     }
-    if (!MoveFile(tmpserver, testagentd))
+
+    if (strcmp(testagentd, full_argv0))
+    {
+        oldtestagentd = get_server_filename(1);
+        if (!oldtestagentd)
+        {
+            set_status(ST_ERROR, "unable to get the backup filename (le=%lu)", GetLastError());
+            goto done;
+        }
+        if (!MoveFile(testagentd, oldtestagentd))
+        {
+            set_status(ST_ERROR, "unable to move the current server file out of the way (le=%lu)", GetLastError());
+            goto done;
+        }
+        if (!MoveFile(argv[0], testagentd))
+        {
+            set_status(ST_ERROR, "unable to move the new server file in place (le=%lu)", GetLastError());
+            MoveFile(oldtestagentd, testagentd);
+            goto done;
+        }
+    }
+
+    tmp_argv0 = argv[0];
+    argv[0] = testagentd;
+    cmdline = build_command_line(argv);
+    argv[0] = tmp_argv0;
+    if (!cmdline)
     {
-        set_status(ST_ERROR, "unable to move the new server file in place (le=%lu)", GetLastError());
-        MoveFile(oldtestagentd, testagentd);
+        set_status(ST_ERROR, "unable to build the new command line");
+        if (oldtestagentd)
+            MoveFile(oldtestagentd, testagentd);
         goto done;
     }
 
     memset(&si, 0, sizeof(si));
     si.cb = sizeof(si);
-    if (!CreateProcessA(testagentd, GetCommandLineA(), NULL, NULL, TRUE,
+    if (!CreateProcessA(testagentd, cmdline, NULL, NULL, TRUE,
                         CREATE_NEW_CONSOLE, NULL, NULL, &si, &pi))
     {
-        set_status(ST_ERROR, "could not run '%s': %lu", GetCommandLineA(), GetLastError());
-        MoveFile(oldtestagentd, testagentd);
+        set_status(ST_ERROR, "could not run '%s': %lu", cmdline, GetLastError());
+        if (oldtestagentd)
+            MoveFile(oldtestagentd, testagentd);
         goto done;
     }
 
@@ -341,6 +513,7 @@ int platform_upgrade(const char* tmpserver, char** argv)
  done:
     free(testagentd);
     free(oldtestagentd);
+    free(cmdline);
     return success;
 }
 
diff --git a/testbot/src/testagentd/testagentd.c b/testbot/src/testagentd/testagentd.c
index 5fd136721..eb93aa203 100644
--- a/testbot/src/testagentd/testagentd.c
+++ b/testbot/src/testagentd/testagentd.c
@@ -39,8 +39,10 @@
  * 1.5:  Add support for upgrading the server.
  * 1.6:  Add the rmchildproc and getcwd RPCs.
  * 1.7:  Add --show-restarts and the setproperty RPC.
+ * 1.8:  Add the restart RPC, server.arg* properties, fix upgrades if >1
+ *       server.
  */
-#define PROTOCOL_VERSION "testagentd 1.7"
+#define PROTOCOL_VERSION "testagentd 1.8"
 
 #define BLOCK_SIZE       65536
 
@@ -94,6 +96,7 @@ enum rpc_ids_t
     RPCID_RMCHILDPROC,
     RPCID_GETCWD,
     RPCID_SETPROPERTY,
+    RPCID_RESTART,
 };
 
 /* This is the RPC currently being processed */
@@ -117,6 +120,7 @@ static const char* rpc_name(uint32_t id)
         "rmchildproc",
         "getcwd",
         "setproperty",
+        "restart",
     };
 
     if (id < sizeof(names) / sizeof(*names))
@@ -1084,6 +1088,63 @@ static void do_setproperty(SOCKET client)
     }
 }
 
+static void do_restart(SOCKET client)
+{
+    uint32_t argc, i;
+    char** argv;
+    int success = 1;
+
+    if (!recv_list_size(client, &argc))
+    {
+        send_error(client);
+        return;
+    }
+    if (argc < 1)
+    {
+        set_status(ST_ERROR, "expected 1 argument or more (got %u)", argc);
+        send_error(client);
+        return;
+    }
+    /* Allocate an extra entry for the trailing NULL pointer */
+    argv = malloc((argc + 1) * sizeof(*argv));
+    if (argv)
+    {
+        memset(argv, 0, (argc + 1) * sizeof(*argv));
+        for (i = 0; i < argc; i++)
+        {
+            if (!recv_string(client, &argv[i]))
+            {
+                success = 0;
+                break;
+            }
+        }
+    }
+    else
+    {
+        set_status(ST_ERROR, "malloc() failed: %s", strerror(errno));
+        skip_entries(client, argc);
+        success = 0;
+    }
+
+    if (success)
+        success = platform_upgrade(server_argv[0], argc, argv);
+
+    /* Free all the memory */
+    for (i = 0; i < argc; i++)
+        free(argv[i]);
+    free(argv);
+
+    if (success)
+    {
+        send_list_size(client, 0);
+        /* Tell the main loop to quit */
+        broken = 1;
+        quit = 1;
+    }
+    else
+        send_error(client);
+}
+
 static void do_upgrade(SOCKET client)
 {
     static const char *filename = "testagentd.tmp";
@@ -1112,7 +1173,12 @@ static void do_upgrade(SOCKET client)
     }
 
     if (success)
-        success = platform_upgrade(filename, server_argv);
+    {
+        char* current_argv0 = server_argv[0];
+        server_argv[0] = (char*)filename;
+        success = platform_upgrade(current_argv0, server_argc, server_argv);
+        server_argv[0] = current_argv0;
+    }
 
     if (success)
     {
@@ -1231,6 +1297,8 @@ static void process_rpc(SOCKET client)
     case RPCID_SETPROPERTY:
         do_setproperty(client);
         break;
+    case RPCID_RESTART:
+        do_restart(client);
     default:
         do_unknown(client, rpcid);
     }
-- 
2.20.1