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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Apr 27, 2023, 13:45 UTC
Message-ID: <dfd693ce-a9dc-6a8c-d16a-0573bc37b93e@jeffhostetler.com>
In-Reply-To: <48d44c06-34ba-0a1f-dd0d-7d66bd8dfcb9@jeffhostetler.com>

On 4/26/23 4:33 PM, Jeff Hostetler wrote: ...

Show 18 quoted lines

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

Show 44 quoted lines

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