On Wed, Mar 19, 2014 at 2:50 PM, Stefan Zager <sza...@chromium.org> wrote:
> On Wed, Mar 19, 2014 at 12:30 AM, Duy Nguyen <pclo...@gmail.com> wrote:
>> On Wed, Mar 19, 2014 at 7:46 AM,  <sza...@chromium.org> wrote:
>>> This adds a Windows implementation of pread.  Note that it is NOT
>>> safe to intersperse calls to read() and pread() on a file
>>> descriptor.  According to the ReadFile spec, using the 'overlapped'
>>> argument should not affect the implicit position pointer of the
>>> descriptor.  Experiments have shown that this is, in fact, a lie.
>> If I understand it correctly, new pread() is added because
>> compat/pread.c does not work because of some other read() in between?
>> Where are those read() (I can only see one in index-pack.c, but there
>> could be some hidden read()..)
> I *think* it's the call to fixup_pack_header_footer(), but I'm not 100% sure.
> I suppose it would be possible to fix the immediate problem just by
> using one fd per thread, without a new pread implementation.  But it
> seems better overall to have a pread() implementation that is
> thread-safe as long as read() and pread() aren't interspersed; and
> then convert all existing read() calls to pread().  That would be a
> good follow-up patch...

I still don't understand how compat/pread.c does not work with pack_fd
per thread. I don't have Windows to test, but I forced compat/pread.c
on on Linux with similar pack_fd changes and it worked fine, helgrind
only complained about progress.c.

A pread() implementation that is thread-safe with condition sounds
like an invite for trouble later. And I don't think converting read()
to pread() is a good idea. Platforms that rely on pread() will hit
first because of more use of compat/pread.c. read() seeks while
pread() does not, so we have to audit more code..
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to