Re: What's cooking in git.git (Apr 2012, #06; Sun, 15)

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Apr 19, 2012, 13:31 UTC
Message-ID: <CABPQNSb9BSfmGu2bYtZUndgkTom=r+Ho_CKsDr7wo=jfxiXEjA@mail.gmail.com>
In-Reply-To: <CACsJy8DDLmKkBMW_P8RyGTRw=i6OKWZNf-SHoGkVWnJgfK7cVQ@mail.gmail.com>

On Thu, Apr 19, 2012 at 3:18 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

Show 13 quoted lines

> On Thu, Apr 19, 2012 at 7:58 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> This approach has the problem that file-operations apart from pread
>> might (at least in theory) modify the position. To prevent that, we'd
>> either need to use the same locking-mechanism as the CRT use, or use
>> ReadFile with an OVERLAPPED struct, which allows us specify the offset
>> explicitly. The latter seems better to me, and should look something
>> like this (note: untested):
>
> Yeah. I read about ReadFile [1] but dismissed it when I got to async
> i/o mode. Reading again, sync i/o ReadFile with OVERLAPPED struct
> should work fine. It's not clear though if file offset is changed
> (pread man page says it does not change).
>

A quick test shows that it does not: ---8<--- $ cat test.c #include <stdio.h> #include <windows.h> #include <errno.h> #include <fcntl.h>

ssize_t mingw_pread(int fd, void *buf, size_t size, off_t offset)
{
       OVERLAPPED overlapped = { 0 };
       DWORD ret;

       HANDLE fh = (HANDLE)_get_osfhandle(fd);
       if (fh == INVALID_HANDLE_VALUE) {
               errno = EBADF;
               return -1;
       }

       overlapped.Offset = (DWORD)offset;
       overlapped.OffsetHigh = (DWORD)(offset >> 32);

       if (!ReadFile(fh, buf, size, &ret, &overlapped)) {
               /* errno = err_win_to_posix(GetLastError()); */
               return -1;
       }

       return ret;
}

int main(int argc, const char *argv[])
{
        int i, fd = open(__FILE__, O_RDONLY);
        for (i = 0; i < 2; ++i) {
                char buf[11] = {0};
                mingw_pread(fd, buf, 10, 0);
                printf("buf = '%10s'\n", buf);
        }
        return 0;
}

$ gcc test.c -o test.exe && ./test.exe test.c: In function 'mingw_pread': test.c:18: warning: right shift count >= width of type buf = '#include <' buf = '#include <' ---8<---

So this looks fine to me.

> Also this approach deals with Windows only. There's still another
> NO_PREAD user, HP-UX something, and NO_PREAD comment mentions cygwin
> before v1.5.22. I personally don't care, just wanted to point out.

Yeah. Other platforms are still an issue. You didn't address those either in your patch, even though it would be possible to modify it to deal with them by checking the NO_PREAD and NO_PTHREADS defines.

But they would still have the problem with the file-pointer racing for non-pread operations. Perhaps simply disabling threading is the better choice for these?