From 29e66773c8ffe9069364c4c025c413e11be87440 Mon Sep 17 00:00:00 2001 From: Nick Bolton Date: Tue, 16 Jan 2024 17:16:26 +0000 Subject: [PATCH] Reset error state before calling Process32Next (#7328) * Convenience script to install daemon * Use -p when re-launching with runas to keep window open * Align new script with PEP 8 * Add more args for flexibility * Add task to reinstall Windows daemon * Fixed mistake in continue message * Close task window after running script * Reset error state before calling Process32Next * Update CL * Increase Windows CI timeout * Bump timeout again due to choco --- .github/workflows/job-test-windows.yml | 2 +- .vscode/tasks.json | 6 ++ ChangeLog | 1 + scripts/windows_daemon.py | 93 ++++++++++++++++++++++++++ src/lib/platform/MSWindowsSession.cpp | 11 ++- 5 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 scripts/windows_daemon.py diff --git a/.github/workflows/job-test-windows.yml b/.github/workflows/job-test-windows.yml index 44a3284df..7cb7aea83 100644 --- a/.github/workflows/job-test-windows.yml +++ b/.github/workflows/job-test-windows.yml @@ -13,7 +13,7 @@ concurrency: jobs: test-windows: runs-on: windows-latest - timeout-minutes: 10 + timeout-minutes: 20 env: GIT_COMMIT: ${{ github.sha }} diff --git a/.vscode/tasks.json b/.vscode/tasks.json index ec81b42bc..5e0bfdcac 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -8,6 +8,12 @@ "targets": ["all"], "preset": "${command:cmake.activeBuildPresetName}", "group": "build" + }, + { + "label": "reinstall windows daemon", + "type": "shell", + "command": "python scripts/windows_daemon.py", + "presentation": { "close": true } } ] } diff --git a/ChangeLog b/ChangeLog index cba517c1c..7a31ed330 100644 --- a/ChangeLog +++ b/ChangeLog @@ -26,6 +26,7 @@ Tasks: - #7325 Add timeout to all GitHub workflows - #7326 Restore lpDesktop assignment in Windows daemon - #7327 Only use Ninja to build on Windows +- #7328 Reset error state before calling Process32Next # 1.14.6 diff --git a/scripts/windows_daemon.py b/scripts/windows_daemon.py new file mode 100644 index 000000000..17d7df942 --- /dev/null +++ b/scripts/windows_daemon.py @@ -0,0 +1,93 @@ +import os +import subprocess +import ctypes +import sys +import argparse +import glob + +BIN_NAME = 'synergyd' +SOURCE_BIN_DIR = os.path.join('build', 'bin') +TARGET_BIN_DIR = 'bin' +SERVICE_NOT_RUNNING_ERROR = 2 + +def main(): + """Entry point for the script.""" + + parser = argparse.ArgumentParser() + parser.add_argument('--pause-on-exit', action='store_true') + parser.add_argument('--source-bin-dir', default=SOURCE_BIN_DIR) + parser.add_argument('--target-bin-dir', default=TARGET_BIN_DIR) + parser.add_argument('--source-bin-name', default=BIN_NAME) + parser.add_argument('--target-bin-name', default=BIN_NAME) + args = parser.parse_args() + + if not is_admin(): + print('Re-launching script as admin') + args = ' '.join(sys.argv[1:]) + command = f'{__file__} --pause-on-exit {args}' + ctypes.windll.shell32.ShellExecuteW(None, 'runas', sys.executable, command, None, 1) + sys.exit() + + try: + reinstall(args.source_bin_dir, args.target_bin_dir, args.source_bin_name, args.target_bin_name) + except Exception as e: + print(f'Error: {e}') + + if (args.pause_on_exit): + input('Press enter to continue...') + +def reinstall(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name): + """Stops the running daemon service, copies files, and reinstalls.""" + + print('Stopping daemon service') + try: + subprocess.run(['net', 'stop', 'synergy'], shell=True, check=True) + except subprocess.CalledProcessError as e: + if (e.returncode == SERVICE_NOT_RUNNING_ERROR): + print('Daemon service not running') + else: + raise e + + copy_bin_files(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name) + + target_bin_file = f'{os.path.join(target_bin_dir, target_bin_name)}.exe' + + print('Removing old daemon service') + subprocess.run([target_bin_file, '/uninstall'], shell=True, check=True) + + print('Installing daemon service') + subprocess.run([target_bin_file, '/install'], shell=True, check=True) + +def copy_bin_files(source_bin_dir, target_bin_dir, source_bin_name, target_bin_name): + + if not os.path.isdir(source_bin_dir): + raise Exception(f'Invalid source bin dir: {source_bin_dir}') + + print(f'Persisting dir: {target_bin_dir}') + os.makedirs(target_bin_dir, exist_ok=True) + + source_bin_glob = f'{source_bin_name}*' + source_files = glob.glob(os.path.join(source_bin_dir, source_bin_glob)) + + if not source_files: + raise Exception(f'No files found in {source_bin_dir} matching {source_bin_glob}') + + for source_file in source_files: + base_name = os.path.basename(source_file) + base_name = base_name.replace(source_bin_name, target_bin_name) + target_file = os.path.join(target_bin_dir, base_name) + print(f'Copying {source_file} to {target_file}') + # use the copy command; shutil.copy gives us a permission denied error. + try: + subprocess.run(['copy', source_file, target_file], shell=True, check=True) + except subprocess.CalledProcessError as e: + print(f'Copy failed: {e}') + +def is_admin(): + """Returns True if the current process has admin privileges.""" + try: + return ctypes.windll.shell32.IsUserAnAdmin() + except ctypes.WinError: + return False + +main() diff --git a/src/lib/platform/MSWindowsSession.cpp b/src/lib/platform/MSWindowsSession.cpp index 7e794be48..6370d8c73 100644 --- a/src/lib/platform/MSWindowsSession.cpp +++ b/src/lib/platform/MSWindowsSession.cpp @@ -158,13 +158,20 @@ MSWindowsSession::updateActiveSession() BOOL MSWindowsSession::nextProcessEntry(HANDLE snapshot, LPPROCESSENTRY32 entry) { + // TODO: issue S3-2021 + // resetting the error state here is acceptable, but having to do so indicates that a + // different win32 function call has failed beforehand. we should always check for errors + // after each win32 function call. + SetLastError(0); + BOOL gotEntry = Process32Next(snapshot, entry); if (!gotEntry) { DWORD err = GetLastError(); - if (err != ERROR_NO_MORE_FILES) { - // only worry about error if it's not the end of the snapshot + // only throw if it's not the end of the snapshot, if not the 'no more files' error + // then it's probably something serious. + if (err != ERROR_NO_MORE_FILES) { LOG((CLOG_ERR "could not get next process entry")); throw XArch(new XArchEvalWindows()); }