Page MenuHomePhabricator

[11/N] [libcxx] Hook up a number of operation functions to their windows counterparts
Needs ReviewPublic

Authored by mstorsjo on Tue, Nov 10, 1:46 AM.

Details

Reviewers
amccarth
EricWF
rnk
Group Reviewers
Restricted Project
Summary

Use the corresponding wchar functions, named "_wfunc" instead of "func", where feasible, or reimplement functions with native windows APIs.

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Nov 10, 1:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 10, 1:46 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk resigned from this revision.Tue, Nov 10, 12:26 PM

I have a couple questions to consider inline. If those have already been thought through, then LTGM.

libcxx/src/filesystem/operations.cpp
518

Technically, there's nothing wrong here. It just _feels_ a little sketchy to call GetLastError() twice in a row.

539

Why not implement this with ::DeleteFile()? Does it have to do with waiting until the last handle is closed?

561

I'm mildly concerned about the way files are sometimes manipulated by HANDLE and sometimes by file descriptor.

If fd represents a file in text mode, then the caller needs to be very careful to understand that length must be the _byte_ offset, not the _char_ offset into the file.

573

Consider also MOVEFILE_WRITE_THROUGH for the case that it is indeed copied and then (probably) deleted.

mstorsjo added inline comments.Thu, Nov 12, 2:20 PM
libcxx/src/filesystem/operations.cpp
518

Yeah, it's not ideal, but then again, making a separate version of the set_errno() function that takes an error code as input, just for this case, also feels a bit superfluous (as the first call to GetLastError() shouldn't clobber the first one). Or would a default parameter, like set_errno(int err = GetLastError()) work?

539

Haven't considered/tested that, I can try that and run it over the testcases to see if it works.

The reason I ended up with this, was that I first just called _wremove(), followed by a _wrmdir() if the first failed and the path is a directory - as that was the most obvious path forward when porting over from the posix remove() function. That worked mostly, but failed in some symlink cases, and then I peeked at how the MS STL (which has the same license as libcxx, fwiw) implemented it, and I did the same.

561

File descriptors here are only an internal detail, fwiw, they're only used within the __copy_file function. But good catch about text vs binary mode; I guess the calls to FileDescriptor::create_with_status need to be amended with O_BINARY.

573

Sounds like a good thing to have, I can add that. These flags were based on what the MS STL use, and I didn't see that flag used there though.

mstorsjo marked 3 inline comments as done.Fri, Nov 13, 3:29 AM
mstorsjo added inline comments.
libcxx/src/filesystem/operations.cpp
518

Changed set_errno() to take the error code as a parameter, with the parameter set by default to GetLastError().

539

I tried with this implementation, but this still fails on removing symlinks to directories:

int remove(const wchar_t *path) {
  if (DeleteFileW(path))
    return 0;
  int e = GetLastError();
  if (e == ERROR_ACCESS_DENIED) {
    if (is_directory(path)) {
      if (!RemoveDirectoryW(path)) 
        return set_errno();
      return 0;
    }
  }
  return set_errno(e);
}
561

Adding O_BINARY at the callsites that open file descriptors.

573

Adding that flag as it sounds beneficial, despite MS STL not using it.

mstorsjo updated this revision to Diff 305080.Fri, Nov 13, 3:43 AM

Made symlink_file_dir pass the inspected error code to set_errno(), passing O_BINARY to the open() function, passing the MOVEFILE_WRITE_THROUGH flag to MoveFileExW.