The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
I tested the patch at hand, and it performs as expected. Files larger than 4GB can be imported.
Steps:
0) create a csv-file that is sufficiently big (>4GB), and one that is small. Use these files to test.
1a) Attempt to import the small file using devel-version.
1b) EXPECTED: success, ACTUAL: success
2a) Attempt to import the big file using devel-version.
2b) EXPECTED: failure, ACTUAL: failure
3) Apply patch and build new version
4a) Attempt to import the small file using patched-version.
4b) EXPECTED: success, ACTUAL: success
4a) Attempt to import the big file using patched-version.
4b) EXPECTED: success, ACTUAL: success
The code looks sensible, it is easy to read and follow. The code uses appropriate win32 functions to perform the task.
Code calculates file size using the following method: buf->st_size = ((__int64) fiData.nFileSizeHigh) << 32 |
(__int64)(fiData.nFileSizeLow);
The hard coded constant 32 is fine, nFileSizeHigh is defined as a DWORD in the Win32 API, which is a 32 bit unsigned
integer.There is no need to a dynamic calculation.
There are minor "nit-picks" that I would change if it were my code, but do not change the functionality of the code.
1)
if (GetFileAttributes(name) == INVALID_FILE_ATTRIBUTES)
{
errno = ENOENT;
return -1;
}
Here I would call _dosmaperr(GetLastError()) instead, just to take account of the possibility that some other error
occurred. Following this change there are slight inconsistency in the order of "CloseHandle(hFile), errno = ENOENT;
return-1" and "_dosmaperr(GetLastError()); CloseHandle(hFile); return -1". I would prefer consistent ordering, but that
isnot important.
The new status of this patch is: Ready for Committer
