Re: [PATCH] mmap implementation for mingw.

[Vasyl' Vavrychuk <vvavrychuk@gmail.com>]

Hello Johannes,

Thank you for your comments. It's my first patch on git so please be kind:)
At the weekend I hope I will find time to work on this.

 > Vasyl Vavrychuk schrieb:
 >
 >> Here is simple and restricted implementation of mmap using
 >> CreateFileMapping, MapViewOfFile.
 >>
 > Thanks. Sign-off?
Will be.

 > Did you notice any differences with this? Or is this change just
 > because-we-can?
Not yet.

 > It doesn't pass the test suite, for example t5301-sliding-window.sh
 > fails.
I will investigate.

 >
 >> --- a/compat/mingw.c
 >> +++ b/compat/mingw.c
 >> @@ -994,3 +994,30 @@ void mingw_open_html(const char *unixpath)
 >> printf("Launching default browser to display HTML ...\n");
 >> ShellExecute(NULL, "open", htmlpath, NULL, "\\", 0);
 >> }
 >> +
 >> +void *mingw_mmap(void *start, size_t length, int prot, int flags,
 >> int fd,
 >> off_t offset)
 >> +{
 >> +	HANDLE handle;
 >> +
 >> +	if (start != NULL || !(flags & MAP_PRIVATE))
 >> +		die("Invalid usage of mingw_mmap");
 > I tend to use this idiom:
 >
 > return errno = EINVAL,
 > error("Invalid usage of mingw_mmap");
Interesting.

 >> +	if (offset % getpagesize() != 0)
 >> +		die("Offset does not match the memory allocation granularity");
 > This is dangerous. Because on MinGW getpagesize() is hard-coded to
 > 0x1000. getpagesize() does not consult GetSystemInfo(). Just skip the
 > check; MapViewOfFile() will report the error later anyway. Or better
 > carefully compute a suitable offset and adjust the length accordingly.
I haven't known about hardcoded getpagesize(). Ajusting will be good but 
it will not solve problem with hardcoded
getpagesize(). Maybe write own getpagesize() based on GetSystemInfo()?

 >
 >> +
 >> +	handle = CreateFileMapping((HANDLE)_get_osfhandle(fd), NULL,
 >> PAGE_WRITECOPY,
 >> +			0, 0, NULL);
 >> +
 >> +	if (handle != NULL) {
 >> +		start = MapViewOfFile(handle, FILE_MAP_COPY, 0, offset,
 >> length);
 >> +		CloseHandle(handle);
 >> +	}
 >> +
 >> +	return start;
 > Upon failure you should return MAP_FAILED, not NULL.
OK

 >> --- a/git-compat-util.h
 >> +++ b/git-compat-util.h
 >> @@ -175,10 +175,12 @@ static inline const char *skip_prefix(const
 >> char *str,
 >> const char *prefix)
 >> #define MAP_FAILED ((void*)-1)
 >> #endif
 >> +#ifndef __MINGW32__
 >> #define mmap git_mmap
 >> #define munmap git_munmap
 >> extern void *git_mmap(void *start, size_t length, int prot, int
 >> flags, int fd,
 >> off_t offset);
 >> extern int git_munmap(void *start, size_t length);
 >> +#endif
 >> /* This value must be multiple of (pagesize * 2) */ #define
 >> DEFAULT_PACKED_GIT_WINDOW_SIZE (1 * 1024 * 1024)
 >>
 > This is inside #ifdef NO_MMAP ... #else section. Isn't that a bit
 > strange? I.e. we say NO_MMAP, but then we do have mmap() now?
 >
Agree