# [PATCH 0/2] Fixup for js/mingw-o-append 21 messages from 2018-09-07 to 2018-09-11. Participants: Jeff Hostetler via GitGitGadget, Jeff Hostetler, Johannes Sixt, Sebastian Schuberth, Junio C Hamano. Thread: https://gitlist.dev/t/49301 ## Jeff Hostetler via GitGitGadget, 2018-09-07 18:19 Subject: [PATCH 0/2] Fixup for js/mingw-o-append Message-ID: URL: https://gitlist.dev/e/pull.35.git.gitgitgadget%40gmail.com ``` The recent change mingw O_APPEND change breaks writing to named pipes on Windows. The first commit adds a new test to confirm the breakage and the second commit fixes the problem. These could be squashed together or we can just keep the fix and omit the test if that would be better. d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND The new mingw_open_append() routine successfully opens the client side of the named pipe, but the first write() to it fails with EBADF. Adding the FILE_WRITE_DATA corrects the problem. Signed-off-by: Jeff Hostetler jeffhost@microsoft.com [jeffhost@microsoft.com] Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: peff@peff.net Jeff Hostetler (2): t0051: test GIT_TRACE to a windows named pipe mingw: fix mingw_open_append to work with named pipes Makefile | 1 + compat/mingw.c | 2 +- t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 6 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh base-commit: d641097589160eb795127d8dbcb14c877c217b60 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/35 -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-07 18:19 Subject: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe Message-ID: <03453cb521ae57475d046024f5840991553410d6.1536344387.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/03453cb521ae57475d046024f5840991553410d6.1536344387.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Create a test-tool helper to create the server side of a windows named pipe, wait for a client connection, and copy data written to the pipe to stdout. Create t0051 test to route GIT_TRACE output of a command to a named pipe using the above test-tool helper. Signed-off-by: Jeff Hostetler --- Makefile | 1 + t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 5 files changed, 96 insertions(+) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh diff --git a/Makefile b/Makefile index e4b503d259..b1b934d295 100644 --- a/Makefile +++ b/Makefile @@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-wildmatch.o +TEST_BUILTINS_OBJS += test-windows-named-pipe.o TEST_BUILTINS_OBJS += test-write-cache.o TEST_PROGRAMS_NEED_X += test-dump-fsmonitor diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 805a45de9c..7a9764cd5c 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -41,6 +41,9 @@ static struct test_cmd cmds[] = { { "subprocess", cmd__subprocess }, { "urlmatch-normalization", cmd__urlmatch_normalization }, { "wildmatch", cmd__wildmatch }, +#ifdef GIT_WINDOWS_NATIVE + { "windows-named-pipe", cmd__windows_named_pipe }, +#endif { "write-cache", cmd__write_cache }, }; diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 7116ddfb94..01c34fe5e7 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); int cmd__wildmatch(int argc, const char **argv); +#ifdef GIT_WINDOWS_NATIVE +int cmd__windows_named_pipe(int argc, const char **argv); +#endif int cmd__write_cache(int argc, const char **argv); #endif diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c new file mode 100644 index 0000000000..b4b752b01a --- /dev/null +++ b/t/helper/test-windows-named-pipe.c @@ -0,0 +1,72 @@ +#include "test-tool.h" +#include "git-compat-util.h" +#include "strbuf.h" + +#ifdef GIT_WINDOWS_NATIVE +static const char *usage_string = ""; + +#define TEST_BUFSIZE (4096) + +int cmd__windows_named_pipe(int argc, const char **argv) +{ + const char *filename; + struct strbuf pathname = STRBUF_INIT; + int err; + HANDLE h; + BOOL connected; + char buf[TEST_BUFSIZE + 1]; + + if (argc < 2) + goto print_usage; + filename = argv[1]; + if (strchr(filename, '/') || strchr(filename, '\\')) + goto print_usage; + strbuf_addf(&pathname, "//./pipe/%s", filename); + + /* + * Create a single instance of the server side of the named pipe. + * This will allow exactly one client instance to connect to it. + */ + h = CreateNamedPipeA( + pathname.buf, + PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, + PIPE_UNLIMITED_INSTANCES, + TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL); + if (h == INVALID_HANDLE_VALUE) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "CreateNamedPipe failed: %s\n", + strerror(err)); + return err; + } + + connected = ConnectNamedPipe(h, NULL) + ? TRUE + : (GetLastError() == ERROR_PIPE_CONNECTED); + if (!connected) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "ConnectNamedPipe failed: %s\n", + strerror(err)); + CloseHandle(h); + return err; + } + + while (1) { + DWORD nbr; + BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL); + if (!success || nbr == 0) + break; + buf[nbr] = 0; + + write(1, buf, nbr); + } + + DisconnectNamedPipe(h); + CloseHandle(h); + return 0; + +print_usage: + fprintf(stderr, "usage: %s %s\n", argv[0], usage_string); + return 1; +} +#endif diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh new file mode 100755 index 0000000000..10ac92d225 --- /dev/null +++ b/t/t0051-windows-named-pipe.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +test_description='Windows named pipes' + +. ./test-lib.sh + +test_expect_success MINGW 'o_append write to named pipe' ' + GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && + { test-tool windows-named-pipe t0051 >actual 2>&1 & } && + pid=$! && + sleep 1 && + GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning && + wait $pid && + test_cmp expect actual +' + +test_done -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-07 18:19 Subject: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/f433937d55974b75750cfc7d579a6a56109259a4.1536344387.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Signed-off-by: Jeff Hostetler --- compat/mingw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 858ca14a57..ef03bbe5d2 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) * FILE_SHARE_WRITE is required to permit child processes * to append to the file. */ - handle = CreateFileW(wfilename, FILE_APPEND_DATA, + handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA, FILE_SHARE_WRITE | FILE_SHARE_READ, NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) -- gitgitgadget ``` ## Jeff Hostetler, 2018-09-07 18:36 Subject: Re: [PATCH 0/2] Fixup for js/mingw-o-append Message-ID: <5103f96f-109a-f070-94a4-82e199f222e3@jeffhostetler.com> URL: https://gitlist.dev/e/5103f96f-109a-f070-94a4-82e199f222e3%40jeffhostetler.com In-Reply-To: ``` GitGitGadget botched the CCs when I submitted this. Replying here to add them. Sorry, Jeff https://github.com/gitgitgadget/gitgitgadget/issues/35 On 9/7/2018 2:19 PM, Jeff Hostetler via GitGitGadget wrote: > The recent change mingw O_APPEND change breaks writing to named pipes on > Windows. The first commit adds a new test to confirm the breakage and the > second commit fixes the problem. These could be squashed together or we can > just keep the fix and omit the test if that would be better. > > d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND > > The new mingw_open_append() routine successfully opens the client side of > the named pipe, but the first write() to it fails with EBADF. Adding the > FILE_WRITE_DATA corrects the problem. > > Signed-off-by: Jeff Hostetler jeffhost@microsoft.com > [jeffhost@microsoft.com] > > Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: > peff@peff.net > > Jeff Hostetler (2): > t0051: test GIT_TRACE to a windows named pipe > mingw: fix mingw_open_append to work with named pipes > > Makefile | 1 + > compat/mingw.c | 2 +- > t/helper/test-tool.c | 3 ++ > t/helper/test-tool.h | 3 ++ > t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ > t/t0051-windows-named-pipe.sh | 17 +++++++ > 6 files changed, 97 insertions(+), 1 deletion(-) > create mode 100644 t/helper/test-windows-named-pipe.c > create mode 100755 t/t0051-windows-named-pipe.sh > > > base-commit: d641097589160eb795127d8dbcb14c877c217b60 > Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/35 > ``` ## Johannes Sixt, 2018-09-08 09:26 Subject: Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: <1c524f56-2021-a961-168f-e5c6d7914ec2@kdbg.org> URL: https://gitlist.dev/e/1c524f56-2021-a961-168f-e5c6d7914ec2%40kdbg.org In-Reply-To: ``` Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget: > From: Jeff Hostetler > > Signed-off-by: Jeff Hostetler > --- > compat/mingw.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index 858ca14a57..ef03bbe5d2 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) > * FILE_SHARE_WRITE is required to permit child processes > * to append to the file. > */ > - handle = CreateFileW(wfilename, FILE_APPEND_DATA, > + handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA, > FILE_SHARE_WRITE | FILE_SHARE_READ, > NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); > if (handle == INVALID_HANDLE_VALUE) > I did not go with this version because the documentation https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants says: FILE_APPEND_DATA: For a file object, the right to append data to the file. (For local files, write operations will not overwrite existing data if this flag is specified without FILE_WRITE_DATA.) [...] which could be interpreted as: Only if FILE_WRITE_DATA is not set, we have the guarantee that existing data in local files is not overwritten, i.e., new data is appended atomically. Is this interpretation too narrow and we do get atomicity even when FILE_WRITE_DATA is set? -- Hannes ``` ## Johannes Sixt, 2018-09-08 18:31 Subject: Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/f207bc28-a303-5d63-e9f4-da8e4d466bd5%40kdbg.org In-Reply-To: <1c524f56-2021-a961-168f-e5c6d7914ec2@kdbg.org> ``` Am 08.09.2018 um 11:26 schrieb Johannes Sixt: > Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget: >> diff --git a/compat/mingw.c b/compat/mingw.c >> index 858ca14a57..ef03bbe5d2 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const >> *wfilename, int oflags, ...) >>        * FILE_SHARE_WRITE is required to permit child processes >>        * to append to the file. >>        */ >> -    handle = CreateFileW(wfilename, FILE_APPEND_DATA, >> +    handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA, >>               FILE_SHARE_WRITE | FILE_SHARE_READ, >>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); >>       if (handle == INVALID_HANDLE_VALUE) >> > > I did not go with this version because the documentation > https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants > says: > > FILE_APPEND_DATA: For a file object, the right to append data to the > file. (For local files, write operations will not overwrite existing > data if this flag is specified without FILE_WRITE_DATA.) [...] > > which could be interpreted as: Only if FILE_WRITE_DATA is not set, we > have the guarantee that existing data in local files is not overwritten, > i.e., new data is appended atomically. > > Is this interpretation too narrow and we do get atomicity even when > FILE_WRITE_DATA is set? Here is are some comments on stackoverflow which let me think that FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic: https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249 -- Hannes ``` ## Sebastian Schuberth, 2018-09-09 07:28 Subject: Re: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe Message-ID: <3b6960e4-df19-a00e-c8f9-e456ecf43ef7@gmail.com> URL: https://gitlist.dev/e/3b6960e4-df19-a00e-c8f9-e456ecf43ef7%40gmail.com In-Reply-To: <03453cb521ae57475d046024f5840991553410d6.1536344387.git.gitgitgadget@gmail.com> ``` On 9/7/2018 8:19 PM, Jeff Hostetler via GitGitGadget wrote: > +test_expect_success MINGW 'o_append write to named pipe' ' Shouldn't this be "test_expect_failure" here, and then be changed to "test_expect_success" by your second patch? -- Sebastian Schuberth ``` ## Jeff Hostetler, 2018-09-10 13:21 Subject: Re: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe Message-ID: URL: https://gitlist.dev/e/cb27d0de-66ca-5b00-7bbb-0ba7041fbeb8%40jeffhostetler.com In-Reply-To: <3b6960e4-df19-a00e-c8f9-e456ecf43ef7@gmail.com> ``` On 9/9/2018 3:28 AM, Sebastian Schuberth wrote: > On 9/7/2018 8:19 PM, Jeff Hostetler via GitGitGadget wrote: > >> +test_expect_success MINGW 'o_append write to named pipe' ' > > Shouldn't this be "test_expect_failure" here, and then be changed to > "test_expect_success" by your second patch? > > yes. thanks! Jeff ``` ## Jeff Hostetler, 2018-09-10 15:44 Subject: Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: <0d38ec8e-3f4b-c0fb-ba6f-e2cef39e4db4@jeffhostetler.com> URL: https://gitlist.dev/e/0d38ec8e-3f4b-c0fb-ba6f-e2cef39e4db4%40jeffhostetler.com In-Reply-To: ``` On 9/8/2018 2:31 PM, Johannes Sixt wrote: > Am 08.09.2018 um 11:26 schrieb Johannes Sixt: >> Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget: >>> diff --git a/compat/mingw.c b/compat/mingw.c >>> index 858ca14a57..ef03bbe5d2 100644 >>> --- a/compat/mingw.c >>> +++ b/compat/mingw.c >>> @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const >>> *wfilename, int oflags, ...) >>>        * FILE_SHARE_WRITE is required to permit child processes >>>        * to append to the file. >>>        */ >>> -    handle = CreateFileW(wfilename, FILE_APPEND_DATA, >>> +    handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA, >>>               FILE_SHARE_WRITE | FILE_SHARE_READ, >>>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); >>>       if (handle == INVALID_HANDLE_VALUE) >>> >> >> I did not go with this version because the documentation >> https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants >> says: >> >> FILE_APPEND_DATA: For a file object, the right to append data to the >> file. (For local files, write operations will not overwrite existing >> data if this flag is specified without FILE_WRITE_DATA.) [...] >> >> which could be interpreted as: Only if FILE_WRITE_DATA is not set, we >> have the guarantee that existing data in local files is not >> overwritten, i.e., new data is appended atomically. >> >> Is this interpretation too narrow and we do get atomicity even when >> FILE_WRITE_DATA is set? > > Here is are some comments on stackoverflow which let me think that > FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic: > > https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249 > > > -- Hannes Yeah, this whole thing is a little under-documented for my tastes. Let's leave it as you have it. I'll re-roll with a fix to route named pipes to the existing _wopen() code. thanks Jeff ``` ## Junio C Hamano, 2018-09-10 16:42 Subject: Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/xmqq5zzduwwe.fsf%40gitster-ct.c.googlers.com In-Reply-To: <0d38ec8e-3f4b-c0fb-ba6f-e2cef39e4db4@jeffhostetler.com> ``` Jeff Hostetler writes: > Yeah, this whole thing is a little under-documented for my tastes. > Let's leave it as you have it. I'll re-roll with a fix to route > named pipes to the existing _wopen() code. OK, I have these two patches in 'next', but let's postpone it for now. Windows port will be built with extra topics over what we have a the release anyway, so your improved version may become part of that and then can come back to benefit us in the next cycle ;-) Thanks. ``` ## Jeff Hostetler, 2018-09-10 16:55 Subject: Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/d415b6c9-9ce6-a7bb-a5db-53b11ce2cccb%40jeffhostetler.com In-Reply-To: ``` On 9/10/2018 12:42 PM, Junio C Hamano wrote: > Jeff Hostetler writes: > >> Yeah, this whole thing is a little under-documented for my tastes. >> Let's leave it as you have it. I'll re-roll with a fix to route >> named pipes to the existing _wopen() code. > > OK, I have these two patches in 'next', but let's postpone it for > now. Windows port will be built with extra topics over what we have > a the release anyway, so your improved version may become part of > that and then can come back to benefit us in the next cycle ;-) > > Thanks. > That's fine. This can easily wait until after 2.19.0. Thanks Jeff ``` ## Jeff Hostetler via GitGitGadget, 2018-09-10 17:05 Subject: [PATCH v2 0/2] Fixup for js/mingw-o-append Message-ID: URL: https://gitlist.dev/e/pull.35.v2.git.gitgitgadget%40gmail.com In-Reply-To: ``` The recent change mingw O_APPEND change breaks writing to named pipes on Windows. The first commit adds a new test to confirm the breakage and the second commit fixes the problem. These could be squashed together or we can just keep the fix and omit the test if that would be better. d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND The new mingw_open_append() routine successfully opens the client side of the named pipe, but the first write() to it fails with EBADF. Adding the FILE_WRITE_DATA corrects the problem. Signed-off-by: Jeff Hostetler jeffhost@microsoft.com [jeffhost@microsoft.com] Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: peff@peff.net Jeff Hostetler (2): t0051: test GIT_TRACE to a windows named pipe mingw: fix mingw_open_append to work with named pipes Makefile | 1 + compat/mingw.c | 38 ++++++++++++++-- t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 6 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh base-commit: d641097589160eb795127d8dbcb14c877c217b60 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/35 Range-diff vs v1: 1: 03453cb521 ! 1: ecb30eb47c t0051: test GIT_TRACE to a windows named pipe @@ -140,7 +140,7 @@ + +. ./test-lib.sh + -+test_expect_success MINGW 'o_append write to named pipe' ' ++test_expect_failure MINGW 'o_append write to named pipe' ' + GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && + { test-tool windows-named-pipe t0051 >actual 2>&1 & } && + pid=$! && 2: f433937d55 < -: ---------- mingw: fix mingw_open_append to work with named pipes -: ---------- > 2: f0361dd306 mingw: fix mingw_open_append to work with named pipes -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-10 17:05 Subject: [PATCH v2 1/2] t0051: test GIT_TRACE to a windows named pipe Message-ID: URL: https://gitlist.dev/e/ecb30eb47c3746444bf30b88329589143db1704c.1536599118.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Create a test-tool helper to create the server side of a windows named pipe, wait for a client connection, and copy data written to the pipe to stdout. Create t0051 test to route GIT_TRACE output of a command to a named pipe using the above test-tool helper. Signed-off-by: Jeff Hostetler --- Makefile | 1 + t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 5 files changed, 96 insertions(+) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh diff --git a/Makefile b/Makefile index e4b503d259..b1b934d295 100644 --- a/Makefile +++ b/Makefile @@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-wildmatch.o +TEST_BUILTINS_OBJS += test-windows-named-pipe.o TEST_BUILTINS_OBJS += test-write-cache.o TEST_PROGRAMS_NEED_X += test-dump-fsmonitor diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 805a45de9c..7a9764cd5c 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -41,6 +41,9 @@ static struct test_cmd cmds[] = { { "subprocess", cmd__subprocess }, { "urlmatch-normalization", cmd__urlmatch_normalization }, { "wildmatch", cmd__wildmatch }, +#ifdef GIT_WINDOWS_NATIVE + { "windows-named-pipe", cmd__windows_named_pipe }, +#endif { "write-cache", cmd__write_cache }, }; diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 7116ddfb94..01c34fe5e7 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); int cmd__wildmatch(int argc, const char **argv); +#ifdef GIT_WINDOWS_NATIVE +int cmd__windows_named_pipe(int argc, const char **argv); +#endif int cmd__write_cache(int argc, const char **argv); #endif diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c new file mode 100644 index 0000000000..b4b752b01a --- /dev/null +++ b/t/helper/test-windows-named-pipe.c @@ -0,0 +1,72 @@ +#include "test-tool.h" +#include "git-compat-util.h" +#include "strbuf.h" + +#ifdef GIT_WINDOWS_NATIVE +static const char *usage_string = ""; + +#define TEST_BUFSIZE (4096) + +int cmd__windows_named_pipe(int argc, const char **argv) +{ + const char *filename; + struct strbuf pathname = STRBUF_INIT; + int err; + HANDLE h; + BOOL connected; + char buf[TEST_BUFSIZE + 1]; + + if (argc < 2) + goto print_usage; + filename = argv[1]; + if (strchr(filename, '/') || strchr(filename, '\\')) + goto print_usage; + strbuf_addf(&pathname, "//./pipe/%s", filename); + + /* + * Create a single instance of the server side of the named pipe. + * This will allow exactly one client instance to connect to it. + */ + h = CreateNamedPipeA( + pathname.buf, + PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, + PIPE_UNLIMITED_INSTANCES, + TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL); + if (h == INVALID_HANDLE_VALUE) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "CreateNamedPipe failed: %s\n", + strerror(err)); + return err; + } + + connected = ConnectNamedPipe(h, NULL) + ? TRUE + : (GetLastError() == ERROR_PIPE_CONNECTED); + if (!connected) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "ConnectNamedPipe failed: %s\n", + strerror(err)); + CloseHandle(h); + return err; + } + + while (1) { + DWORD nbr; + BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL); + if (!success || nbr == 0) + break; + buf[nbr] = 0; + + write(1, buf, nbr); + } + + DisconnectNamedPipe(h); + CloseHandle(h); + return 0; + +print_usage: + fprintf(stderr, "usage: %s %s\n", argv[0], usage_string); + return 1; +} +#endif diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh new file mode 100755 index 0000000000..e3c36341a0 --- /dev/null +++ b/t/t0051-windows-named-pipe.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +test_description='Windows named pipes' + +. ./test-lib.sh + +test_expect_failure MINGW 'o_append write to named pipe' ' + GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && + { test-tool windows-named-pipe t0051 >actual 2>&1 & } && + pid=$! && + sleep 1 && + GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning && + wait $pid && + test_cmp expect actual +' + +test_done -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-10 17:05 Subject: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/f0361dd306d19fa741c813885d240e041dc09a7a.1536599118.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Signed-off-by: Jeff Hostetler --- compat/mingw.c | 38 ++++++++++++++++++++++++++++++++--- t/t0051-windows-named-pipe.sh | 2 +- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 858ca14a57..f87376b26a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode) return ret; } +/* + * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA + * is documented in [1] as opening a writable file handle in append mode. + * (It is believed that) this is atomic since it is maintained by the + * kernel unlike the O_APPEND flag which is racily maintained by the CRT. + * + * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants + * + * This trick does not appear to work for named pipes. Instead it creates + * a named pipe client handle that cannot be written to. Callers should + * just use the regular _wopen() for them. (And since client handle gets + * bound to a unique server handle, it isn't really an issue.) + */ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) { HANDLE handle; @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) return errno = err_win_to_posix(GetLastError()), -1; + /* * No O_APPEND here, because the CRT uses it only to reset the - * file pointer to EOF on write(); but that is not necessary - * for a file created with FILE_APPEND_DATA. + * file pointer to EOF before each write(); but that is not + * necessary (and may lead to races) for a file created with + * FILE_APPEND_DATA. */ fd = _open_osfhandle((intptr_t)handle, O_BINARY); if (fd < 0) @@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) return fd; } +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) +/* + * Does the pathname map to the local named pipe filesystem? + * That is, does it have a "//./pipe/" prefix? + */ +static int mingw_is_local_named_pipe_path(const char *filename) +{ + return (IS_SBS(filename[0]) && + IS_SBS(filename[1]) && + filename[2] == '.' && + IS_SBS(filename[3]) && + !strncasecmp(filename+4, "pipe", 4) && + IS_SBS(filename[8]) && + filename[9]); +} +#undef IS_SBS + int mingw_open (const char *filename, int oflags, ...) { typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...); @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...) if (filename && !strcmp(filename, "/dev/null")) filename = "nul"; - if (oflags & O_APPEND) + if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename)) open_fn = mingw_open_append; else open_fn = _wopen; diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh index e3c36341a0..10ac92d225 100755 --- a/t/t0051-windows-named-pipe.sh +++ b/t/t0051-windows-named-pipe.sh @@ -4,7 +4,7 @@ test_description='Windows named pipes' . ./test-lib.sh -test_expect_failure MINGW 'o_append write to named pipe' ' +test_expect_success MINGW 'o_append write to named pipe' ' GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && { test-tool windows-named-pipe t0051 >actual 2>&1 & } && pid=$! && -- gitgitgadget ``` ## Johannes Sixt, 2018-09-10 19:45 Subject: Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/a309396f-bb33-477d-5d92-a98699f5a856%40kdbg.org In-Reply-To: ``` Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget: > diff --git a/compat/mingw.c b/compat/mingw.c > index 858ca14a57..f87376b26a 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode) > return ret; > } > > +/* > + * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA > + * is documented in [1] as opening a writable file handle in append mode. > + * (It is believed that) this is atomic since it is maintained by the > + * kernel unlike the O_APPEND flag which is racily maintained by the CRT. > + * > + * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants > + * > + * This trick does not appear to work for named pipes. Instead it creates > + * a named pipe client handle that cannot be written to. Callers should > + * just use the regular _wopen() for them. (And since client handle gets > + * bound to a unique server handle, it isn't really an issue.) > + */ > static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) > { > HANDLE handle; > @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) > NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); > if (handle == INVALID_HANDLE_VALUE) > return errno = err_win_to_posix(GetLastError()), -1; > + > /* > * No O_APPEND here, because the CRT uses it only to reset the > - * file pointer to EOF on write(); but that is not necessary > - * for a file created with FILE_APPEND_DATA. > + * file pointer to EOF before each write(); but that is not > + * necessary (and may lead to races) for a file created with > + * FILE_APPEND_DATA. > */ > fd = _open_osfhandle((intptr_t)handle, O_BINARY); > if (fd < 0) > @@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) > return fd; > } > > +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) > +/* > + * Does the pathname map to the local named pipe filesystem? > + * That is, does it have a "//./pipe/" prefix? > + */ > +static int mingw_is_local_named_pipe_path(const char *filename) > +{ > + return (IS_SBS(filename[0]) && > + IS_SBS(filename[1]) && > + filename[2] == '.' && > + IS_SBS(filename[3]) && > + !strncasecmp(filename+4, "pipe", 4) && > + IS_SBS(filename[8]) && > + filename[9]); > +} > +#undef IS_SBS > + > int mingw_open (const char *filename, int oflags, ...) > { > typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...); > @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...) > if (filename && !strcmp(filename, "/dev/null")) > filename = "nul"; > > - if (oflags & O_APPEND) > + if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename)) > open_fn = mingw_open_append; > else > open_fn = _wopen; This looks reasonable. I wonder which part of the code uses local named pipes. Is it downstream in Git for Windows or one of the topics in flight? -- Hannes ``` ## Jeff Hostetler, 2018-09-10 20:07 Subject: Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/eedde82a-dedb-88ba-4bb9-fbcfdd7f1b62%40jeffhostetler.com In-Reply-To: ``` On 9/10/2018 3:45 PM, Johannes Sixt wrote: > Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget: >> diff --git a/compat/mingw.c b/compat/mingw.c >> index 858ca14a57..f87376b26a 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode) >>       return ret; >>   } >> +/* >> + * Calling CreateFile() using FILE_APPEND_DATA and without >> FILE_WRITE_DATA >> + * is documented in [1] as opening a writable file handle in append >> mode. >> + * (It is believed that) this is atomic since it is maintained by the >> + * kernel unlike the O_APPEND flag which is racily maintained by the >> CRT. >> + * >> + * [1] >> https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants >> >> + * >> + * This trick does not appear to work for named pipes.  Instead it >> creates >> + * a named pipe client handle that cannot be written to.  Callers should >> + * just use the regular _wopen() for them.  (And since client handle >> gets >> + * bound to a unique server handle, it isn't really an issue.) >> + */ >>   static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) >>   { >>       HANDLE handle; >> @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const >> *wfilename, int oflags, ...) >>               NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); >>       if (handle == INVALID_HANDLE_VALUE) >>           return errno = err_win_to_posix(GetLastError()), -1; >> + >>       /* >>        * No O_APPEND here, because the CRT uses it only to reset the >> -     * file pointer to EOF on write(); but that is not necessary >> -     * for a file created with FILE_APPEND_DATA. >> +     * file pointer to EOF before each write(); but that is not >> +     * necessary (and may lead to races) for a file created with >> +     * FILE_APPEND_DATA. >>        */ >>       fd = _open_osfhandle((intptr_t)handle, O_BINARY); >>       if (fd < 0) >> @@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const >> *wfilename, int oflags, ...) >>       return fd; >>   } >> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) >> +/* >> + * Does the pathname map to the local named pipe filesystem? >> + * That is, does it have a "//./pipe/" prefix? >> + */ >> +static int mingw_is_local_named_pipe_path(const char *filename) >> +{ >> +    return (IS_SBS(filename[0]) && >> +        IS_SBS(filename[1]) && >> +        filename[2] == '.'  && >> +        IS_SBS(filename[3]) && >> +        !strncasecmp(filename+4, "pipe", 4) && >> +        IS_SBS(filename[8]) && >> +        filename[9]); >> +} >> +#undef IS_SBS >> + >>   int mingw_open (const char *filename, int oflags, ...) >>   { >>       typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, >> ...); >> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, >> ...) >>       if (filename && !strcmp(filename, "/dev/null")) >>           filename = "nul"; >> -    if (oflags & O_APPEND) >> +    if ((oflags & O_APPEND) && >> !mingw_is_local_named_pipe_path(filename)) >>           open_fn = mingw_open_append; >>       else >>           open_fn = _wopen; > > This looks reasonable. Thanks for the review. > > I wonder which part of the code uses local named pipes. Is it downstream > in Git for Windows or one of the topics in flight? > > -- Hannes I'm wanting to use them as a tracing target option in my trace2 series currently in progress. Jeff ``` ## Junio C Hamano, 2018-09-10 22:00 Subject: Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: URL: https://gitlist.dev/e/xmqqbm95rp27.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Johannes Sixt writes: >> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) I think you already have mingw_is_dir_sep() and its shorter alias is_dir_sep() available to you. >> +/* >> + * Does the pathname map to the local named pipe filesystem? >> + * That is, does it have a "//./pipe/" prefix? >> + */ >> +static int mingw_is_local_named_pipe_path(const char *filename) There is no need to prefix mingw_ to this function that is file local static. Isn't is_local_named_pipe() descriptive and unique enough? >> +{ >> + return (IS_SBS(filename[0]) && >> + IS_SBS(filename[1]) && >> + filename[2] == '.' && >> + IS_SBS(filename[3]) && >> + !strncasecmp(filename+4, "pipe", 4) && >> + IS_SBS(filename[8]) && >> + filename[9]); >> +} >> +#undef IS_SBS It is kind-of surprising that there hasn't been any existing need for a helper function that would allow us to write this function like so: static int is_local_named_pipe(const char *path) { return path_is_in_directory(path, "//./pipe/"); } Not a suggestion to add such a thing; as long as we know there is no other codepath that would benefit from having one, a generalization like that can and should wait. >> int mingw_open (const char *filename, int oflags, ...) >> { >> typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...); >> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...) >> if (filename && !strcmp(filename, "/dev/null")) >> filename = "nul"; >> - if (oflags & O_APPEND) >> + if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename)) >> open_fn = mingw_open_append; >> else >> open_fn = _wopen; > > This looks reasonable. > > I wonder which part of the code uses local named pipes. Is it > downstream in Git for Windows or one of the topics in flight? > > -- Hannes ``` ## Jeff Hostetler, 2018-09-11 14:25 Subject: Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: <26f3d84d-cfa4-bb03-a2f8-d2e0fe5ad12e@jeffhostetler.com> URL: https://gitlist.dev/e/26f3d84d-cfa4-bb03-a2f8-d2e0fe5ad12e%40jeffhostetler.com In-Reply-To: ``` On 9/10/2018 6:00 PM, Junio C Hamano wrote: > Johannes Sixt writes: > >>> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) > > I think you already have mingw_is_dir_sep() and its shorter alias > is_dir_sep() available to you. good catch. thanks. >>> +/* >>> + * Does the pathname map to the local named pipe filesystem? >>> + * That is, does it have a "//./pipe/" prefix? >>> + */ >>> +static int mingw_is_local_named_pipe_path(const char *filename) > > There is no need to prefix mingw_ to this function that is file > local static. Isn't is_local_named_pipe() descriptive and unique > enough? right. will do. >>> +{ >>> + return (IS_SBS(filename[0]) && >>> + IS_SBS(filename[1]) && >>> + filename[2] == '.' && >>> + IS_SBS(filename[3]) && >>> + !strncasecmp(filename+4, "pipe", 4) && >>> + IS_SBS(filename[8]) && >>> + filename[9]); >>> +} >>> +#undef IS_SBS > > It is kind-of surprising that there hasn't been any existing need > for a helper function that would allow us to write this function > like so: > > static int is_local_named_pipe(const char *path) > { > return path_is_in_directory(path, "//./pipe/"); > } > > Not a suggestion to add such a thing; as long as we know there is no > other codepath that would benefit from having one, a generalization > like that can and should wait. Yeah, I don't think we need something that general just yet. Named pipes exist in a special namespace using the UNC/network-share syntax (rather than the DOS drive-letter syntax), and we don't do much with UNC paths yet. Perhaps, later we could have something to try splitting a UNC path into , , and and then have is_local_named_pipe() verify the first 2 are as expected. Or have a function like is_path_in_volume(path, server, volume) that does the tests without cutting up strings and allocating. We could do either, but I don't think we need to be that general yet. Thanks, Jeff ``` ## Jeff Hostetler via GitGitGadget, 2018-09-11 20:05 Subject: [PATCH v3 0/2] Fixup for js/mingw-o-append Message-ID: URL: https://gitlist.dev/e/pull.35.v3.git.gitgitgadget%40gmail.com In-Reply-To: ``` The recent change mingw O_APPEND change breaks writing to named pipes on Windows. The first commit adds a new test to confirm the breakage and the second commit fixes the problem. These could be squashed together or we can just keep the fix and omit the test if that would be better. d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND The new mingw_open_append() routine successfully opens the client side of the named pipe, but the first write() to it fails with EBADF. Adding the FILE_WRITE_DATA corrects the problem. Signed-off-by: Jeff Hostetler jeffhost@microsoft.com [jeffhost@microsoft.com] Cc: j6t@kdbg.orgCc: johannes.schindelin@gmx.deCc: gitster@pobox.comCc: peff@peff.net Jeff Hostetler (2): t0051: test GIT_TRACE to a windows named pipe mingw: fix mingw_open_append to work with named pipes Makefile | 1 + compat/mingw.c | 36 +++++++++++++-- t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 6 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh base-commit: d641097589160eb795127d8dbcb14c877c217b60 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-35/jeffhostetler/fixup-mingw-o-append-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/35 Range-diff vs v2: 1: ecb30eb47c = 1: ecb30eb47c t0051: test GIT_TRACE to a windows named pipe 2: f0361dd306 ! 2: 5592300ca5 mingw: fix mingw_open_append to work with named pipes @@ -46,22 +46,20 @@ return fd; } -+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\')) +/* + * Does the pathname map to the local named pipe filesystem? + * That is, does it have a "//./pipe/" prefix? + */ -+static int mingw_is_local_named_pipe_path(const char *filename) ++static int is_local_named_pipe_path(const char *filename) +{ -+ return (IS_SBS(filename[0]) && -+ IS_SBS(filename[1]) && ++ return (is_dir_sep(filename[0]) && ++ is_dir_sep(filename[1]) && + filename[2] == '.' && -+ IS_SBS(filename[3]) && ++ is_dir_sep(filename[3]) && + !strncasecmp(filename+4, "pipe", 4) && -+ IS_SBS(filename[8]) && ++ is_dir_sep(filename[8]) && + filename[9]); +} -+#undef IS_SBS + int mingw_open (const char *filename, int oflags, ...) { @@ -71,7 +69,7 @@ filename = "nul"; - if (oflags & O_APPEND) -+ if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename)) ++ if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename)) open_fn = mingw_open_append; else open_fn = _wopen; -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-11 20:06 Subject: [PATCH v3 1/2] t0051: test GIT_TRACE to a windows named pipe Message-ID: URL: https://gitlist.dev/e/ecb30eb47c3746444bf30b88329589143db1704c.1536696358.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Create a test-tool helper to create the server side of a windows named pipe, wait for a client connection, and copy data written to the pipe to stdout. Create t0051 test to route GIT_TRACE output of a command to a named pipe using the above test-tool helper. Signed-off-by: Jeff Hostetler --- Makefile | 1 + t/helper/test-tool.c | 3 ++ t/helper/test-tool.h | 3 ++ t/helper/test-windows-named-pipe.c | 72 ++++++++++++++++++++++++++++++ t/t0051-windows-named-pipe.sh | 17 +++++++ 5 files changed, 96 insertions(+) create mode 100644 t/helper/test-windows-named-pipe.c create mode 100755 t/t0051-windows-named-pipe.sh diff --git a/Makefile b/Makefile index e4b503d259..b1b934d295 100644 --- a/Makefile +++ b/Makefile @@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-wildmatch.o +TEST_BUILTINS_OBJS += test-windows-named-pipe.o TEST_BUILTINS_OBJS += test-write-cache.o TEST_PROGRAMS_NEED_X += test-dump-fsmonitor diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 805a45de9c..7a9764cd5c 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -41,6 +41,9 @@ static struct test_cmd cmds[] = { { "subprocess", cmd__subprocess }, { "urlmatch-normalization", cmd__urlmatch_normalization }, { "wildmatch", cmd__wildmatch }, +#ifdef GIT_WINDOWS_NATIVE + { "windows-named-pipe", cmd__windows_named_pipe }, +#endif { "write-cache", cmd__write_cache }, }; diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 7116ddfb94..01c34fe5e7 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); int cmd__wildmatch(int argc, const char **argv); +#ifdef GIT_WINDOWS_NATIVE +int cmd__windows_named_pipe(int argc, const char **argv); +#endif int cmd__write_cache(int argc, const char **argv); #endif diff --git a/t/helper/test-windows-named-pipe.c b/t/helper/test-windows-named-pipe.c new file mode 100644 index 0000000000..b4b752b01a --- /dev/null +++ b/t/helper/test-windows-named-pipe.c @@ -0,0 +1,72 @@ +#include "test-tool.h" +#include "git-compat-util.h" +#include "strbuf.h" + +#ifdef GIT_WINDOWS_NATIVE +static const char *usage_string = ""; + +#define TEST_BUFSIZE (4096) + +int cmd__windows_named_pipe(int argc, const char **argv) +{ + const char *filename; + struct strbuf pathname = STRBUF_INIT; + int err; + HANDLE h; + BOOL connected; + char buf[TEST_BUFSIZE + 1]; + + if (argc < 2) + goto print_usage; + filename = argv[1]; + if (strchr(filename, '/') || strchr(filename, '\\')) + goto print_usage; + strbuf_addf(&pathname, "//./pipe/%s", filename); + + /* + * Create a single instance of the server side of the named pipe. + * This will allow exactly one client instance to connect to it. + */ + h = CreateNamedPipeA( + pathname.buf, + PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE, + PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT, + PIPE_UNLIMITED_INSTANCES, + TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL); + if (h == INVALID_HANDLE_VALUE) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "CreateNamedPipe failed: %s\n", + strerror(err)); + return err; + } + + connected = ConnectNamedPipe(h, NULL) + ? TRUE + : (GetLastError() == ERROR_PIPE_CONNECTED); + if (!connected) { + err = err_win_to_posix(GetLastError()); + fprintf(stderr, "ConnectNamedPipe failed: %s\n", + strerror(err)); + CloseHandle(h); + return err; + } + + while (1) { + DWORD nbr; + BOOL success = ReadFile(h, buf, TEST_BUFSIZE, &nbr, NULL); + if (!success || nbr == 0) + break; + buf[nbr] = 0; + + write(1, buf, nbr); + } + + DisconnectNamedPipe(h); + CloseHandle(h); + return 0; + +print_usage: + fprintf(stderr, "usage: %s %s\n", argv[0], usage_string); + return 1; +} +#endif diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh new file mode 100755 index 0000000000..e3c36341a0 --- /dev/null +++ b/t/t0051-windows-named-pipe.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +test_description='Windows named pipes' + +. ./test-lib.sh + +test_expect_failure MINGW 'o_append write to named pipe' ' + GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && + { test-tool windows-named-pipe t0051 >actual 2>&1 & } && + pid=$! && + sleep 1 && + GIT_TRACE=//./pipe/t0051 git status >/dev/null 2>warning && + wait $pid && + test_cmp expect actual +' + +test_done -- gitgitgadget ``` ## Jeff Hostetler via GitGitGadget, 2018-09-11 20:06 Subject: [PATCH v3 2/2] mingw: fix mingw_open_append to work with named pipes Message-ID: <5592300ca526422ed3a5cf000e9e574cc97b252d.1536696358.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/5592300ca526422ed3a5cf000e9e574cc97b252d.1536696358.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Jeff Hostetler Signed-off-by: Jeff Hostetler --- compat/mingw.c | 36 ++++++++++++++++++++++++++++++++--- t/t0051-windows-named-pipe.sh | 2 +- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 858ca14a57..18caf21969 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode) return ret; } +/* + * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA + * is documented in [1] as opening a writable file handle in append mode. + * (It is believed that) this is atomic since it is maintained by the + * kernel unlike the O_APPEND flag which is racily maintained by the CRT. + * + * [1] https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants + * + * This trick does not appear to work for named pipes. Instead it creates + * a named pipe client handle that cannot be written to. Callers should + * just use the regular _wopen() for them. (And since client handle gets + * bound to a unique server handle, it isn't really an issue.) + */ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) { HANDLE handle; @@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) return errno = err_win_to_posix(GetLastError()), -1; + /* * No O_APPEND here, because the CRT uses it only to reset the - * file pointer to EOF on write(); but that is not necessary - * for a file created with FILE_APPEND_DATA. + * file pointer to EOF before each write(); but that is not + * necessary (and may lead to races) for a file created with + * FILE_APPEND_DATA. */ fd = _open_osfhandle((intptr_t)handle, O_BINARY); if (fd < 0) @@ -371,6 +386,21 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) return fd; } +/* + * Does the pathname map to the local named pipe filesystem? + * That is, does it have a "//./pipe/" prefix? + */ +static int is_local_named_pipe_path(const char *filename) +{ + return (is_dir_sep(filename[0]) && + is_dir_sep(filename[1]) && + filename[2] == '.' && + is_dir_sep(filename[3]) && + !strncasecmp(filename+4, "pipe", 4) && + is_dir_sep(filename[8]) && + filename[9]); +} + int mingw_open (const char *filename, int oflags, ...) { typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...); @@ -387,7 +417,7 @@ int mingw_open (const char *filename, int oflags, ...) if (filename && !strcmp(filename, "/dev/null")) filename = "nul"; - if (oflags & O_APPEND) + if ((oflags & O_APPEND) && !is_local_named_pipe_path(filename)) open_fn = mingw_open_append; else open_fn = _wopen; diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh index e3c36341a0..10ac92d225 100755 --- a/t/t0051-windows-named-pipe.sh +++ b/t/t0051-windows-named-pipe.sh @@ -4,7 +4,7 @@ test_description='Windows named pipes' . ./test-lib.sh -test_expect_failure MINGW 'o_append write to named pipe' ' +test_expect_success MINGW 'o_append write to named pipe' ' GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 && { test-tool windows-named-pipe t0051 >actual 2>&1 & } && pid=$! && -- gitgitgadget ```