Fixup for js/mingw-o-append

+test_expect_success MINGW 'o_append write to named pipe' '

Quoted 1 line · Jeff Hostetler via GitGitGadget

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

From: Jeff Hostetler <[email protected]>

Signed-off-by Jeff Hostetler <[email protected]>

---
  compat/mingw.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

compat/mingw.c +0 -0

index 858ca14a57..ef03bbe5d2 100644

Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:

Quoted 13 lines

compat/mingw.c +1 -1

index 858ca14a57..ef03bbe5d2 100644
@@ -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);

Quoted 2 lines

      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?

Am 08.09.2018 um 11:26 schrieb Johannes Sixt:

Quoted 23 lines

Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:

Quoted 13 lines

compat/mingw.c +1 -1

index 858ca14a57..ef03bbe5d2 100644
@@ -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);

Quoted 2 lines

      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.

Quoted 1 line

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.

Quoted 3 lines · Jeff Hostetler

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.

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 [email protected]

[[email protected]]

Cc: [email protected]: [email protected]: [email protected]:
[email protected]

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

compat/mingw.c +34 -2

index 858ca14a57..f87376b26a 100644
@@ -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, ...);

Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:

Quoted 74 lines

compat/mingw.c +2 -0

index 858ca14a57..f87376b26a 100644
@@ -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;

compat/mingw.c +4 -2

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

compat/mingw.c +17 -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";

Quoted 6 lines

compat/mingw.c +1 -1

-    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

Quoted 1 line

  +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))

compat/mingw.c static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) · line 394

Quoted 5 lines

compat/mingw.c +5 -0

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

compat/mingw.c static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) · line 404

Quoted 10 lines

compat/mingw.c +10 -0

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

compat/mingw.c int mingw_open (const char *filename, int oflags, ...) · line 425

Quoted 11 lines

int mingw_open (const char *filename, int oflags, ...)
{
typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);

compat/mingw.c +1 -0

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

Quoted 1 line · Johannes Sixt

Quoted 1 line

   +#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.

Quoted 5 lines

compat/mingw.c static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) · line 394

Quoted 5 lines

compat/mingw.c +5 -0

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

Quoted 10 lines

compat/mingw.c static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) · line 404

Quoted 10 lines

compat/mingw.c +10 -0

+{
+	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.