diff mbox series

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

Message ID pull.1503.v2.git.1681155963011.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] fsmonitor: handle differences between Windows named pipe functions | expand

Commit Message

Eric DeCosta April 10, 2023, 7:46 p.m. UTC 
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>
---
    fsmonitor: handle differences between Windows named pipe functions
    
    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.
    
    v1 differs from v0:
    
     * Abandon hashing repo path in favor of simpler solution where the
       leading slash is simply dropped for UNC paths

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1503

Range-diff vs v1:

 1:  c5307928cd7 ! 1:  b41f9bd2e96 fsmonitor: handle differences between Windows named pipe functions
     @@ Metadata
       ## Commit message ##
          fsmonitor: handle differences between Windows named pipe functions
      
     -    CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
     -    embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
     -    returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
     -    with CreateNamedPipeW, exists.
     +    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.
      
     -    For example, this network path is problemmatic:
     -    \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
     +    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
      
     -    In order to work around this issue, rather than using the path to the
     -    worktree directly as the name of the pipe, instead use the hash of the
     -    worktree path.
     +    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 ##
     -@@
     - #include "cache.h"
     -+#include "hex.h"
     - #include "simple-ipc.h"
     - #include "strbuf.h"
     - #include "pkt-line.h"
      @@
       static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
       {
       	int off = 0;
     --	struct strbuf realpath = STRBUF_INIT;
     --
     --	if (!strbuf_realpath(&realpath, path, 0))
     --		return -1;
     -+	int ret = 0;
     -+	git_SHA_CTX sha1ctx;
     -+	struct strbuf real_path = STRBUF_INIT;
     -+	struct strbuf pipe_name = STRBUF_INIT;
     -+	unsigned char hash[GIT_MAX_RAWSZ];
     ++	int real_off = 0;
     + 	struct strbuf realpath = STRBUF_INIT;
       
     --	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
     --	if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
     -+	if (!strbuf_realpath(&real_path, path, 0))
     + 	if (!strbuf_realpath(&realpath, path, 0))
       		return -1;
       
     --	/* Handle drive prefix */
     --	if (wpath[off] && wpath[off + 1] == L':') {
     --		wpath[off + 1] = L'_';
     --		off += 2;
     --	}
     -+	git_SHA1_Init(&sha1ctx);
     -+	git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
     -+	git_SHA1_Final(hash, &sha1ctx);
     -+	strbuf_release(&real_path);
     - 
     --	for (; wpath[off]; off++)
     --		if (wpath[off] == L'/')
     --			wpath[off] = L'\\';
     -+	strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash));
     -+	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
     -+	if (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)
     -+		ret = -1;
     - 
     --	strbuf_release(&realpath);
     --	return 0;
     -+	strbuf_release(&pipe_name);
     -+	return ret;
     - }
     ++	/* 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;
       
     - static enum ipc_active_state get_active_state(wchar_t *pipe_path)
     + 	/* Handle drive prefix */


 compat/simple-ipc/ipc-win32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


base-commit: f285f68a132109c234d93490671c00218066ace9

Comments

Eric DeCosta April 22, 2023, 8 p.m. UTC | #1 
> -----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>
> ---
> fsmonitor: handle differences between Windows named pipe functions
> 
> 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.
> 
> v1 differs from v0:
> 
> * Abandon hashing repo path in favor of simpler solution where the leading
> slash is simply dropped for UNC paths
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-
> 1503%2Fedecosta-mw%2Ffsmonitor_windows-v2 <https://protect-
> us.mimecast.com/s/WUMKCqxoXGIDMzRYtE7lUM?domain=github.com>
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git <https://protect-
> us.mimecast.com/s/meLwCrkpNJSpB16QhjvzbL?domain=github.com>  pr-
> 1503/edecosta-mw/fsmonitor_windows-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1503 <https://protect-
> us.mimecast.com/s/0_e0Cv2w7NFGk24wH5_RA3?domain=github.com>
> 
> Range-diff vs v1:
> 
> 1: c5307928cd7 ! 1: b41f9bd2e96 fsmonitor: handle differences between
> Windows named pipe functions @@ Metadata ## Commit message ##
> fsmonitor: handle differences between Windows named pipe functions
> 
> - CreateNamedPipeW is perfectly happy accepting pipe names with
> seemingly
> - embedded escape charcters (e.g. \b), WaitNamedPipeW is not and
> incorrectly
> - returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully
> created
> - with CreateNamedPipeW, exists.
> + 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.
> 
> - For example, this network path is problemmatic:
> - \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
> + 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
> 
> - In order to work around this issue, rather than using the path to the
> - worktree directly as the name of the pipe, instead use the hash of the
> - worktree path.
> + 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 ##
> -@@
> - #include "cache.h"
> -+#include "hex.h"
> - #include "simple-ipc.h"
> - #include "strbuf.h"
> - #include "pkt-line.h"
> @@
> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
> alloc) { int off = 0;
> -- struct strbuf realpath = STRBUF_INIT;
> --
> -- if (!strbuf_realpath(&realpath, path, 0))
> -- return -1;
> -+ int ret = 0;
> -+ git_SHA_CTX sha1ctx;
> -+ struct strbuf real_path = STRBUF_INIT; struct strbuf pipe_name =
> -+ STRBUF_INIT; unsigned char hash[GIT_MAX_RAWSZ];
> ++ int real_off = 0;
> + struct strbuf realpath = STRBUF_INIT;
> 
> -- off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
> -- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
> -+ if (!strbuf_realpath(&real_path, path, 0))
> + if (!strbuf_realpath(&realpath, path, 0))
> return -1;
> 
> -- /* Handle drive prefix */
> -- if (wpath[off] && wpath[off + 1] == L':') {
> -- wpath[off + 1] = L'_';
> -- off += 2;
> -- }
> -+ git_SHA1_Init(&sha1ctx);
> -+ git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
> -+ git_SHA1_Final(hash, &sha1ctx); strbuf_release(&real_path);
> -
> -- for (; wpath[off]; off++)
> -- if (wpath[off] == L'/')
> -- wpath[off] = L'\\';
> -+ strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash)); off =
> -+ swprintf(wpath, alloc, L"\\\\.\\pipe\\"); if (xutftowcs(wpath + off,
> -+ pipe_name.buf, alloc - off) < 0) ret = -1;
> -
> -- strbuf_release(&realpath);
> -- return 0;
> -+ strbuf_release(&pipe_name);
> -+ return ret;
> - }
> ++ /* 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;
> 
> - static enum ipc_active_state get_active_state(wchar_t *pipe_path)
> + /* Handle drive prefix */
> 
> 
> 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;
> 
> /* 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
Jeff Hostetler April 26, 2023, 8:33 p.m. UTC | #2 
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
Jeff Hostetler April 27, 2023, 1:45 p.m. UTC | #3 
On 4/26/23 4:33 PM, Jeff Hostetler wrote:
...
>>>
>>> + /* 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() ?

On second thought, you might actually need the second slash
in //./pipe//server/mount/whatever so that the GfW installer
can find remote repo path to CWD and stop the daemon.

Testing will tell here.

Jeff

> 
> 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
Eric DeCosta May 8, 2023, 8:30 p.m. UTC | #4 
> -----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
Jeff Hostetler May 15, 2023, 9:50 p.m. UTC | #5 
Sorry this got lost in my inbox.

On 5/8/23 4:30 PM, Eric DeCosta wrote:
> 
>> -----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.

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
>
diff mbox series

Patch

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;
 
 	/* Handle drive prefix */