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 is not important.
The new status of this patch is: Ready for Committer
