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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
