> -----Original Message----- > From: Johannes Schindelin [mailto:Johannes.Schindelin@xxxxxx] > Sent: Monday, September 18, 2017 10:25 AM > To: Ben Peart <Ben.Peart@xxxxxxxxxxxxx> > Cc: David.Turner@xxxxxxxxxxxx; avarab@xxxxxxxxx; > christian.couder@xxxxxxxxx; git@xxxxxxxxxxxxxxx; gitster@xxxxxxxxx; > pclouds@xxxxxxxxx; peff@xxxxxxxx > Subject: Re: [PATCH v6 12/12] fsmonitor: add a performance test > > 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"); > Thanks Johannes, I'll fix that. > 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 patch is pretty hefty already. How about you push this capability upstream and I take advantage of it in a later patch. :) This would be the patch (you can also cherry-pick it from > 25c4dc3a73352e72e995594cf1b4afa46e93d040 in > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub. > com%2Fdscho%2Fgit&data=02%7C01%7CBen.Peart%40microsoft.com%7C96 > 4027bdc1f34a033c1f08d4fea1056e%7C72f988bf86f141af91ab2d7cd011db47 > %7C1%7C0%7C636413414914282865&sdata=jyvu6G7myRY9UA1XxWx2tDZ% > 2BWsIWqLTRMT8WfzEGe5g%3D&reserved=0): > > -- snip -- > From 25c4dc3a73352e72e995594cf1b4afa46e93d040 Mon Sep 17 00:00:00 > 2001 > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > 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@xxxxxxx> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > --- > 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@xxxxxx> > 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
