Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler <git@jeffhostetler.com>]

On 4/22/23 4:00 PM, Eric DeCosta wrote:
> 
>> -----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>
[...]
>>
>>
>> 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


>>
>> /* 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