Re: [PATCH] mmap implementation for mingw.

[Johannes Sixt <j.sixt@viscovery.net>]

Vasyl Vavrychuk schrieb:
> Here is simple and restricted implementation of mmap using CreateFileMapping, 
> MapViewOfFile.

Thanks. Sign-off?

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

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

> --- 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");

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

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

> --- 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?

-- Hannes