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.. -- Duy -- 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
