Re: [PATCH v6 12/12] fsmonitor: add a performance test

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Ben,

sorry for not catching this earlier:

On Fri, 15 Sep 2017, Ben Peart wrote:

> [...]
> +
> +int cmd_dropcaches(void)
> +{
> +	HANDLE hProcess = GetCurrentProcess();
> +	HANDLE hToken;
> +	int status;
> +
> +	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
> +		return error("Can't open current process token");
> +
> +	if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
> +		return error("Can't get SeProfileSingleProcessPrivilege");
> +
> +	CloseHandle(hToken);
> +
> +	HMODULE ntdll = LoadLibrary("ntdll.dll");

Git's source code still tries to abide by C90, and for simplicity's sake,
this extends to the Windows-specific part. Therefore, the `ntdll` variable
needs to be declared at the beginning of the function (I do agree that it
makes for better code to reduce the scope of variables, but C90 simply did
not allow variables to be declared in the middle of functions).

I wanted to send a patch address this in the obvious way, but then I
encountered these lines:

> +	DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) =
> +		(DWORD(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, "NtSetSystemInformation");
> +	if (!NtSetSystemInformation)
> +		return error("Can't get function addresses, wrong Windows version?");

It turns out that we have seen this plenty of times in Git for Windows'
fork, so much so that we came up with a nice helper to make this all a bit
more robust and a bit more obvious, too: the DECLARE_PROC_ADDR and
INIT_PROC_ADDR helpers in compat/win32/lazyload.h.

Maybe this would be the perfect excuse to integrate this patch into
upstream Git? This would be the patch (you can also cherry-pick it from
25c4dc3a73352e72e995594cf1b4afa46e93d040 in https://github.com/dscho/git):

-- snip --
From 25c4dc3a73352e72e995594cf1b4afa46e93d040 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Tue, 10 Jan 2017 23:14:20 +0100
Subject: [PATCH] Win32: simplify loading of DLL functions

Dynamic loading of DLL functions is duplicated in several places in Git
for Windows' source code.

This patch adds a pair of macros to simplify the process: the
DECLARE_PROC_ADDR(<dll>, <return-type>, <function-name>,
...<function-parameter-types>...) macro to be used at the beginning of a
code block, and the INIT_PROC_ADDR(<function-name>) macro to call before
using the declared function. The return value of the INIT_PROC_ADDR()
call has to be checked; If it is NULL, the function was not found in the
specified DLL.

Example:

        DECLARE_PROC_ADDR(kernel32.dll, BOOL, CreateHardLinkW,
                          LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES);

        if (!INIT_PROC_ADDR(CreateHardLinkW))
                return error("Could not find CreateHardLinkW() function";

	if (!CreateHardLinkW(source, target, NULL))
		return error("could not create hardlink from %S to %S",
			     source, target);
	return 0;

Signed-off-by: Karsten Blees <blees@dcon.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/win32/lazyload.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 compat/win32/lazyload.h

diff --git a/compat/win32/lazyload.h b/compat/win32/lazyload.h
new file mode 100644
index 00000000000..91c10dad2fb
--- /dev/null
+++ b/compat/win32/lazyload.h
@@ -0,0 +1,44 @@
+#ifndef LAZYLOAD_H
+#define LAZYLOAD_H
+
+/* simplify loading of DLL functions */
+
+struct proc_addr {
+	const char *const dll;
+	const char *const function;
+	FARPROC pfunction;
+	unsigned initialized : 1;
+};
+
+/* Declares a function to be loaded dynamically from a DLL. */
+#define DECLARE_PROC_ADDR(dll, rettype, function, ...) \
+	static struct proc_addr proc_addr_##function = \
+	{ #dll, #function, NULL, 0 }; \
+	static rettype (WINAPI *function)(__VA_ARGS__)
+
+/*
+ * Loads a function from a DLL (once-only).
+ * Returns non-NULL function pointer on success.
+ * Returns NULL + errno == ENOSYS on failure.
+ */
+#define INIT_PROC_ADDR(function) \
+	(function = get_proc_addr(&proc_addr_##function))
+
+static inline void *get_proc_addr(struct proc_addr *proc)
+{
+	/* only do this once */
+	if (!proc->initialized) {
+		HANDLE hnd;
+		proc->initialized = 1;
+		hnd = LoadLibraryExA(proc->dll, NULL,
+				     LOAD_LIBRARY_SEARCH_SYSTEM32);
+		if (hnd)
+			proc->pfunction = GetProcAddress(hnd, proc->function);
+	}
+	/* set ENOSYS if DLL or function was not found */
+	if (!proc->pfunction)
+		errno = ENOSYS;
+	return proc->pfunction;
+}
+
+#endif
-- snap --

With this patch, this fixup to your patch would make things compile (you
can also cherry-pick d05996fb61027512b8ab31a36c4a7a677dea11bb from my
fork):

-- snipsnap --
From d05996fb61027512b8ab31a36c4a7a677dea11bb Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 18 Sep 2017 14:56:40 +0200
Subject: [PATCH] fixup! fsmonitor: add a performance test

---
 t/helper/test-drop-caches.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/t/helper/test-drop-caches.c b/t/helper/test-drop-caches.c
index 717079865cb..b27358528f7 100644
--- a/t/helper/test-drop-caches.c
+++ b/t/helper/test-drop-caches.c
@@ -1,6 +1,7 @@
 #include "git-compat-util.h"
 
 #if defined(GIT_WINDOWS_NATIVE)
+#include "compat/win32/lazyload.h"
 
 int cmd_sync(void)
 {
@@ -82,6 +83,9 @@ int cmd_dropcaches(void)
 	HANDLE hProcess = GetCurrentProcess();
 	HANDLE hToken;
 	int status;
+	SYSTEM_MEMORY_LIST_COMMAND command;
+	DECLARE_PROC_ADDR(ntll,
+			  DWORD, NtSetSystemInformation, INT, PVOID, ULONG);
 
 	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
 		return error("Can't open current process token");
@@ -91,16 +95,10 @@ int cmd_dropcaches(void)
 
 	CloseHandle(hToken);
 
-	HMODULE ntdll = LoadLibrary("ntdll.dll");
-	if (!ntdll)
-		return error("Can't load ntdll.dll, wrong Windows
 		version?");
-
-	DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) =
-		(DWORD(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll,
 		"NtSetSystemInformation");
-	if (!NtSetSystemInformation)
+	if (!INIT_PROC_ADDR(NtSetSystemInformation))
 		return error("Can't get function addresses, wrong Windows version?");
 
-	SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
+	command = MemoryPurgeStandbyList;
 	status = NtSetSystemInformation(
 		SystemMemoryListInformation,
 		&command,
@@ -111,8 +109,6 @@ int cmd_dropcaches(void)
 	else if (status != STATUS_SUCCESS)
 		error("Unable to execute the memory list command %d", status);
 
-	FreeLibrary(ntdll);
-
 	return status;
 }
 
-- 
2.14.1.windows.1.510.g0cb6d35d23