dart:io | Add Link.targetSync on all platforms.

dart:io | Add Link.targetSync on all platforms.


BUG=

Committed: https://code.google.com/p/dart/source/detail?r=20195

[Bill Hesse, 2013-03-18 09:05:54]

https://codereview.chromium.org/12812010/diff/15/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/15/runtime/bin/file_win.cc#newc...
runtime/bin/file_win.cc:321: target += 4;
We really need to copy the data out here, because the string is not guaranteed
to be null-terminated, and the place we want to write the null is not guaranteed
to be in-bounds.

Will fix and upload new version, or let StringUtils::WideToUtf8 take a length.

https://codereview.chromium.org/12812010/diff/15/sdk/lib/io/link.dart
File sdk/lib/io/link.dart (right):

https://codereview.chromium.org/12812010/diff/15/sdk/lib/io/link.dart#newcode85
sdk/lib/io/link.dart:85: * Get the target of the link. Returns a future that
completes with
Gets

[Søren Gjesse, 2013-03-18 12:28:03]

LGTM, with comments

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file.cc#newcod...
runtime/bin/file.cc:458: Dart_Handle err = DartUtils::NewDartOSError();
For Windows there are cases where there is no direct OS error here.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:311: if (CloseHandle(dir_handle) == 0) return NULL;
Here it seems like we are clearing the last error before returning NULL. Don't
we need a GetLastError/SetLastError here?

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:313: if (reparse_data_buffer->ReparseTag !=
IO_REPARSE_TAG_MOUNT_POINT) {
Here GetLastError will return NOERROR, so we get an OSError with an error code
of 0.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:321: if (target_length <= 4 || wcsncmp(L"\\??\\",
target, 4) != 0) return NULL;
Ditto.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:322: int utf8_length = WideCharToMultiByte(CP_UTF8,
Indentation.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:329: NULL);
Can this call fail?

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:340: free(utf8_target);
Set an error code?

[Bill Hesse, 2013-03-18 16:13:27]

Made many changes to file_win.cc, so please take a look at it.  Thanx.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file.cc#newcod...
runtime/bin/file.cc:458: Dart_Handle err = DartUtils::NewDartOSError();
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> For Windows there are cases where there is no direct OS error here.

Fixed.  All returns of NULL from windows set an OS error or have nothing between
the system call and the return.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:311: if (CloseHandle(dir_handle) == 0) return NULL;
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> Here it seems like we are clearing the last error before returning NULL. Don't
> we need a GetLastError/SetLastError here?

Yes.  Fixed.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:313: if (reparse_data_buffer->ReparseTag !=
IO_REPARSE_TAG_MOUNT_POINT) {
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> Here GetLastError will return NOERROR, so we get an OSError with an error code
> of 0.

Now we are returning Not a Reparse Point.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:321: if (target_length <= 4 || wcsncmp(L"\\??\\",
target, 4) != 0) return NULL;
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> Ditto.

Changed.  Now, we just skip removing \??\ if it isn't there.  This is the right
behavior for symbolic links.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:329: NULL);
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> Can this call fail?
If it fails, it returns 0, and the next call to it will fail as well.

https://codereview.chromium.org/12812010/diff/8001/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:340: free(utf8_target);
On 2013/03/18 12:28:03, Søren Gjesse wrote:
> Set an error code?

Done.

https://codereview.chromium.org/12812010/diff/3003/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/3003/runtime/bin/file_win.cc#ne...
runtime/bin/file_win.cc:313: DWORD error = GetLastError();
This code occurs, slightly differently, 4 times, but all refactoring just makes
it uglier.

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:320: DWORD error = GetLastError();
Trying to combine the two error cases here gets ugly fast.

[Søren Gjesse, 2013-03-19 09:40:29]

lgtm

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:346: // Remove "\??\" from beginning of target.
Don't you mean \\?\ here?

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:347: if (target_length > 4 && wcsncmp(L"\\??\\", target,
4) == 0) {
L"\\\\?\\"

[Bill Hesse, 2013-03-19 10:34:03]

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/12812010/diff/10002/runtime/bin/file_win.cc#n...
runtime/bin/file_win.cc:346: // Remove "\??\" from beginning of target.
On 2013/03/19 09:40:29, Søren Gjesse wrote:
> Don't you mean \\?\ here?

No, I really mean \??\.  This is what is used in junctions, not \\?\.  I thought
it was a mistake too, but this is what it is.  This is also in the Chromium
code.