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

[Eric DeCosta <edecosta@mathworks.com>]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Wednesday, April 26, 2023 4:34 PM
> To: Eric DeCosta <edecosta@mathworks.com>; Eric DeCosta via GitGitGadget
> <gitgitgadget@gmail.com>; git@vger.kernel.org
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows
> named pipe functions
> 
> 
> 
> 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() ?
> 
Attempts to add additional backslashes after \\.\pipe\\ are apparently
ignored. The name of the local pipe always ends up looking like this:

for UNC paths:
  \\.\pipe\\some-server\some-dir\some-repo
  
and for local paths:
 \\.\pipe\\C_\some-dir\some-repo
  
Thus for a UNC path of //some-server/some-dir/some-repo the simplest thing to
do is to skip the first slash.

> 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?
> 
I have used both PowerShell and Process Explorer and see similar results.

> 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.
> 
In regards to the GfW installer, if the daemon is running against
a network mounted repo it reports the following:

Could not stop FSMonitor daemon in some-server\some-dir\some-repo
(output: , errors: fatal cannot change to 'some-server\some-dir\some-repo':
No such file or directory)

It looks like the installer may have to do something like:
look for "<drive letter>_" immediately after "\\.\pipe\\" and if it does not
find it, assume a UNC path.

> 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.
> 
I hear what you are saying, however reporting that information increases
the scope of this change. As this stands right now the advertised feature
of allowing fsmonitor to work on network-mounted sandboxes for Windows
is not working as expected.

-Eric

> 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