[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Enable index-pack threading in msysgit.
From:       Karsten Blees <karsten.blees () gmail ! com>
Date:       2014-03-20 13:54:12
Message-ID: 532AF304.7040301 () gmail ! com
[Download RAW message or body]

Am 19.03.2014 01:46, schrieb szager@chromium.org:
> This adds a Windows implementation of pread.  Note that it is NOT
> safe to intersperse calls to read() and pread() on a file
> descriptor.

This is a bad idea. You're basically fixing the multi-threaded issue twice, while at \
the same time breaking single-threaded read/pread interop on the mingw and msvc \
platform. Users of pread already have to take care that its not thread-safe on some \
platforms, now you're adding another breakage that has to be considered in future \
development.

The mingw_pread implementation in [1] is both thread-safe and allows mixing \
read/pread in single-threaded scenarios, why not use this instead?

[1] http://article.gmane.org/gmane.comp.version-control.git/242120

> 
> http://article.gmane.org/gmane.comp.version-control.git/196042
> 

Duy's patch alone enables multi-threaded index-pack on all platforms (including \
cygwin), so IMO this should be a separate patch.

> +	if (hand == INVALID_HANDLE_VALUE) {
> +		errno = EBADF;
> +		return -1;
> +	}

This check is redundant, ReadFile already ckecks for invalid handles and \
err_win_to_posix converts to EBADF.

> +
> +	LARGE_INTEGER offset_value;
> +	offset_value.QuadPart = offset;
> +
> +	DWORD bytes_read = 0;
> +	OVERLAPPED overlapped = {0};
> +	overlapped.Offset = offset_value.LowPart;
> +	overlapped.OffsetHigh = offset_value.HighPart;
> +	BOOL result = ReadFile(hand, buf, count, &bytes_read, &overlapped);
> +
> +	ssize_t ret = bytes_read;
> +
> +	if (!result && GetLastError() != ERROR_HANDLE_EOF)

According to MSDN docs, ReadFile never fails with ERROR_HANDLE_EOF, or is this \
another case where the documentation is wrong?

"When a synchronous read operation reaches the end of a file, ReadFile returns TRUE \
and sets *lpNumberOfBytesRead to zero."

Karsten
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]