win: restore file position after positional read/write operation #1357
Conversation
|
I've noticed this before, it surfaces as an undocumented aspect of the node fs.read/write calls. The windows implementation can't be atomic, as |
src/win/fs.c
Outdated
| ++index; | ||
| } while (result && index < req->fs.info.nbufs); | ||
|
|
||
| if (restore_position) { |
| } while (result && index < req->fs.info.nbufs); | ||
|
|
||
| if (restore_position) { | ||
| SetFilePointerEx(handle, original_position, NULL, FILE_BEGIN); |
test/test-fs.c
Outdated
| unlink("test_file"); | ||
| loop = uv_default_loop(); | ||
|
|
||
| r = uv_fs_open(loop, &open_req1, "test_file", O_RDWR | O_CREAT, |
There was a problem hiding this comment.
style: please line arguments vertically
| r = uv_fs_open(loop, &open_req1, "test_file", O_RDWR | O_CREAT, | ||
| S_IWUSR | S_IRUSR, NULL); | ||
| ASSERT(r >= 0); | ||
| ASSERT(open_req1.result >= 0); |
There was a problem hiding this comment.
this assert is kinda redundant, since the result of a sync open is the same as the return value.
test/test-fs.c
Outdated
| iov = uv_buf_init(test_buf, sizeof(test_buf)); | ||
| r = uv_fs_write(NULL, &write_req, open_req1.result, &iov, 1, 0, NULL); | ||
| ASSERT(r >= 0); | ||
| ASSERT(write_req.result >= 0); |
test/test-fs.c
Outdated
| iov = uv_buf_init(buf, sizeof(buf)); | ||
| r = uv_fs_read(NULL, &read_req, open_req1.result, &iov, 1, 0, NULL); | ||
| ASSERT(r >= 0); | ||
| ASSERT(read_req.result >= 0); |
|
Updated, PTAL |
File read or write from specified position will move file pointer on Windows but not on POSIX. This makes Windows behave as other supported platforms. Ref: nodejs/node#9671
|
Rebased, PTAL |
|
Ping? |
test/test-fs.c
Outdated
| O_RDWR | O_CREAT, | ||
| S_IWUSR | S_IRUSR, | ||
| NULL); | ||
| ASSERT(r >= 0); |
There was a problem hiding this comment.
Shouldn't r be greater than 0?
There was a problem hiding this comment.
My test is modified fs_file_open_append test, didn't give a second though. I'll update the tests.
test/test-fs.c
Outdated
|
|
||
| iov = uv_buf_init(test_buf, sizeof(test_buf)); | ||
| r = uv_fs_write(NULL, &write_req, open_req1.result, &iov, 1, 0, NULL); | ||
| ASSERT(r >= 0); |
There was a problem hiding this comment.
shouldn't we check the number r == sizeof(test_buf)?
test/test-fs.c
Outdated
|
|
||
| iov = uv_buf_init(buf, sizeof(buf)); | ||
| r = uv_fs_read(NULL, &read_req, open_req1.result, &iov, 1, 0, NULL); | ||
| ASSERT(r >= 0); |
| overlapped_ptr = &overlapped; | ||
| if (SetFilePointerEx(handle, zero_offset, &original_position, | ||
| FILE_CURRENT)) { | ||
| restore_position = 1; |
There was a problem hiding this comment.
On which cases can this fail? Should we note in the documentation that on Windows the offset could actually be modified after calling uv_fs_write() / uv_fs_read()?
There was a problem hiding this comment.
As per MSDN SetFilePointerEx doc this call can fail if the handle is not a file or if it was opened with FILE_FLAG_NO_BUFFERING. We test handle type and libuv does not uses that flag, so I guess we are safe.
|
Updated, PTAL |
santigimeno
left a comment
santigimeno
left a comment
There was a problem hiding this comment.
SGTM though I can't test it locally.
|
@saghul, can I get +1 from you? |
File read or write from specified position will move file pointer on Windows but not on POSIX. This makes Windows behave as other supported platforms. Ref: nodejs/node#9671 PR-URL: #1357 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
|
Landed in 0bd8f5b |
4 participants
File read or write from specified position will move file pointer on Windows but not on POSIX. This makes Windows behave as other supported platforms: for writes/reads with provided offset, file position before operation is saved, and then restored once the operation completes.
Ref: nodejs/node#9671