diff mbox series

[2/2] mingw: fix mingw_open_append to work with named pipes

Message ID f433937d55974b75750cfc7d579a6a56109259a4.1536344387.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fixup for js/mingw-o-append | expand

Commit Message

Linus Arver via GitGitGadget Sept. 7, 2018, 6:19 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Johannes Sixt Sept. 8, 2018, 9:26 a.m. UTC | #1 
Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>   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 Sept. 8, 2018, 6:31 p.m. UTC | #2 
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
Jeff Hostetler Sept. 10, 2018, 3:44 p.m. UTC | #3 
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 Sept. 10, 2018, 4:42 p.m. UTC | #4 
Jeff Hostetler <git@jeffhostetler.com> 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 Sept. 10, 2018, 4:55 p.m. UTC | #5 
On 9/10/2018 12:42 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> 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
diff mbox series

Patch

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)