Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions
- From
- Jeff Hostetler <git@jeffhostetler.com>
- Date
- Apr 26, 2023, 20:33 UTC
- Message-ID
- <48d44c06-34ba-0a1f-dd0d-7d66bd8dfcb9@jeffhostetler.com>
- In-Reply-To
- <BL0PR05MB557134157C3D08D982DEB338D9619@BL0PR05MB5571.namprd05.prod.outlook.com>
On 4/22/23 4:00 PM, Eric DeCosta wrote:
Show 29 quoted lines
> >> -----Original Message----- >> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com> >> Sent: Monday, April 10, 2023 3:46 PM >> To: git@vger.kernel.org >> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler >> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric >> DeCosta <edecosta@mathworks.com> >> Subject: [PATCH v2] fsmonitor: handle differences between Windows named >> pipe functions >> >> From: Eric DeCosta <edecosta@mathworks.com> >> >> The two functions involved with creating and checking for the existance of >> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW >> appear to handle names with leading slashes or backslashes a bit differently. >> >> If a repo resolves to a remote file system with the UNC path of //some- >> server/some-dir/some-repo, CreateNamedPipeW accepts this name and >> creates this named pipe: \\.\pipe\some-server\some-dir\some-repo >> >> However, when the same UNC path is passed to WaitNamedPipeW, it fails >> with ERROR_FILE_NOT_FOUND. >> >> Skipping the leading slash for UNC paths works for both CreateNamedPipeW >> and WaitNamedPipeW. Resulting in a named pipe with the same name as >> above that WaitNamedPipeW is able to correctly find. >> >> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
[...]
Show 24 quoted lines
>>
>>
>> compat/simple-ipc/ipc-win32.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
>> index 997f6144344..632b12e1ab5 100644
>> --- a/compat/simple-ipc/ipc-win32.c
>> +++ b/compat/simple-ipc/ipc-win32.c
>> @@ -19,13 +19,18 @@
>> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
>> alloc) { int off = 0;
>> + int real_off = 0;
>> struct strbuf realpath = STRBUF_INIT;
>>
>> if (!strbuf_realpath(&realpath, path, 0)) return -1;
>>
>> + /* UNC Path, skip leading slash */
>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
>> +
>> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
>> return -1;I haven't had a chance to test this, but this does look like a minimal solution for the pathname confusion in the MSFT APIs.
Do you need to test for realpath.buf[0] and [1] being a forward OR a backslash ?
Should we set real_off to 2 rather than 1 because we already appended a trailing backslash in the swprintf() ?
You should run one of those NPFS directory listing tools to confirm the exact spelling of the pipe matches your expectation here. Yes, if both functions now work, we should be good, but it would be good to confirm your real_off choice, right?
If would be good to (at least interactively) test that the git-for-windows installer can find the path and stop the daemon on an upgrade or uninstall. See Johannes' earlier point.
We should state somewhere that we are running the fsmonitor daemon locally and it is watching a remote file system.
You should run a few stress tests to ensure that the MAX_RDCW_BUF_FALLBACK throttling works and that the daemon doesn't fall behind on a very busy remote file system. (There are SMB/CIFS wire protocol limits. See the source.) (I did test this between the combination of systems that I had, but YMMV.)
During the stress test, it would also be good to test that IO generated by a client process on your local machine to the remote file system is reported, but also that random IO from remote processes on the remote system are seen in the event stream. Again, I tested the combinations of machines that I had available at the time.
Hope this helps, Jeff
Show 13 quoted lines
>> >> /* Handle drive prefix */ >> >> base-commit: f285f68a132109c234d93490671c00218066ace9 >> -- >> gitgitgadget > > Are there any other thoughts about this? > > I believe that this is the simplest change possible that will ensure that > fsmonitor correctly handles network repos. > > -Eric