Sorry this got lost in my inbox.
On 5/8/23 4:30 PM, Eric DeCosta wrote:
-----Original Message-----
From: Jeff Hostetler <git@xxxxxxxxxxxxxxxxx>
Sent: Wednesday, April 26, 2023 4:34 PM
To: Eric DeCosta <edecosta@xxxxxxxxxxxxx>; Eric DeCosta via GitGitGadget
<gitgitgadget@xxxxxxxxx>; git@xxxxxxxxxxxxxxx
Cc: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
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@xxxxxxxxx>
Sent: Monday, April 10, 2023 3:46 PM
To: git@xxxxxxxxxxxxxxx
Cc: Johannes Schindelin <Johannes.Schindelin@xxxxxx>; Jeff Hostetler
<git@xxxxxxxxxxxxxxxxx>; Eric DeCosta <edecosta@xxxxxxxxxxxxx>; Eric
DeCosta <edecosta@xxxxxxxxxxxxx>
Subject: [PATCH v2] fsmonitor: handle differences between Windows
named
pipe functions
From: Eric DeCosta <edecosta@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
[...]
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.
Ok thanks. Just being paranoid...
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.
good. thanks.
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.
ok. perhaps you could log an issue on github.com/git-for-windows/git.git
to describe this.
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.
understood. my only concern was that remotely mounted file systems
didn't get a lot of testing. it worked, but i didn't have resources
to really stress it. but then again, it if falls behind it will
force a resync, so you *shouldn't* get a wrong answer.
Thanks for all your work (and patience with me) on this.
Jeff
-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
