Nice to see somebody filling in the gaps like this.
A few nits based on a superficial reading:
CopyFileA leaks a string. (I know, it did before your change, too.)
copy_file_open_dest's interface comment has the wrong name.
+ for(i = 0; i < sizeof(flags) / sizeof(flags[0]); i++) + { + if(for_write) {
Please use the same whitespace conventions as in the rest of the file (in particular, leave a space after keywords like for and if).
+ if ((h1 = copy_file_open_source(source, copyFlags & COPY_FILE_OPEN_SOURCE_FOR_WRITE)) == INVALID_HANDLE_VALUE)
That line's too long... try to keep them under 80 chars.
+ // FIXME: total_file_size should include the sum of all streams
Use C comments, not C++ comments.
+ DWORD result = progressRoutine(total_file_size, total_bytes_transferred, stream_size, stream_bytes_transferred, + stream_number, CALLBACK_STREAM_SWITCH, h1, h2, appData);
Another instance of line longer than 80 chars. (You have several.)
You don't have a conformance test, nor was there one for CopyFile. Maybe you should consider writing one, and making sure it passes both on Wine and Windows.
Say, what app is this for? - Dan
Sorry for jumping in, but I have some comments as well. Dan, CopyFileA does not leaks a string (note BOOL flags). Instead first call use static TEB-based buffer so this is OK, given that recursion is impossible in this case. Kevin, you are setting file size before copy starts. I'm not sure Windows does so; also how would that work if destination file system does not support sparse files?
Dan Kegel wrote:
Nice to see somebody filling in the gaps like this.
A few nits based on a superficial reading:
CopyFileA leaks a string. (I know, it did before your change, too.)
copy_file_open_dest's interface comment has the wrong name.
- for(i = 0; i < sizeof(flags) / sizeof(flags[0]); i++)
- {
if(for_write) {Please use the same whitespace conventions as in the rest of the file (in particular, leave a space after keywords like for and if).
- if ((h1 = copy_file_open_source(source, copyFlags &
COPY_FILE_OPEN_SOURCE_FOR_WRITE)) == INVALID_HANDLE_VALUE)
That line's too long... try to keep them under 80 chars.
- // FIXME: total_file_size should include the sum of all streams
Use C comments, not C++ comments.
DWORD result = progressRoutine(total_file_size,total_bytes_transferred, stream_size, stream_bytes_transferred,
stream_number,CALLBACK_STREAM_SWITCH, h1, h2, appData);
Another instance of line longer than 80 chars. (You have several.)
You don't have a conformance test, nor was there one for CopyFile. Maybe you should consider writing one, and making sure it passes both on Wine and Windows.
Say, what app is this for?
- Dan
On Tuesday 08 May 2007 2:39 am, Andrey Turkin wrote:
Kevin, you are setting file size before copy starts. I'm not sure Windows does so; also how would that work if destination file system does not support sparse files?
This is what windows does (at least XP)
SetEndOfFile does not create a sparse file, a file must be explicitly declared as sparse. http://msdn2.microsoft.com/en-us/library/aa365566.aspx
On 5/7/07, Andrey Turkin andrey.turkin@gmail.com wrote:
Dan, CopyFileA does not leaks a string (note BOOL flags). Instead first call use static TEB-based buffer so this is OK, given that recursion is impossible in this case.
Whoops! Sorry, I should have remembered that. (And I meant to insert "seems to" before sending, but forgot.) - Dan
On Monday 07 May 2007 11:50 pm, Dan Kegel wrote:
You don't have a conformance test, nor was there one for CopyFile. Maybe you should consider writing one, and making sure it passes both on Wine and Windows.
There is a test for CopyFile in kernel32/tests/file.c, if I can find some time I plan to flesh it out further, but since CopyFile just calls CopyFileEx it is at least partly testing this code.
Say, what app is this for?
Romcenter - http://www.romcenter.com/
On 5/10/07, Kevin Koltzau kevin@plop.org wrote:
On Monday 07 May 2007 11:50 pm, Dan Kegel wrote:
You don't have a conformance test, nor was there one for CopyFile. Maybe you should consider writing one, and making sure it passes both on Wine and Windows.
There is a test for CopyFile in kernel32/tests/file.c
Dang, missed that. (I miss a lot of stuff lately.)
if I can find some time I plan to flesh it out further, but since CopyFile just calls CopyFileEx it is at least partly testing this code.
Yeah, but it doesn't test the feature you needed it for. If Alexandre doesn't accept the patch in a few days, you should probably add a better test, that helps convince him.
Say, what app is this for?
Romcenter - http://www.romcenter.com/
I don't see a bug in bugzilla for that, but a problem is mentioned in http://appdb.winehq.org/appview.php?iVersionId=6666 Does your patch fix that problem?
Filing a good bug report in bugzilla for the problem your patch solves, and then referring to it from your patch, will also help get your patch accepted.
-
Andrey Turkin -
Dan Kegel -
Dan Kegel -
Kevin Koltzau