Make filesystem::rename POSIX compliant by SibiSiddharthan · Pull Request #2062 · microsoft/STL

SibiSiddharthan · 2021-07-15T18:48:26Z

Fixed bugs

Renaming files which are hard-links of each other should be a no-op.
Renaming directories where the new path points to an empty directory
should succeed.

Added test cases as well for the above.

Fixes #2053

SibiSiddharthan · 2021-07-15T19:03:49Z

I have not yet fixed the issue regarding the read-only bit as I needed to clarify something before I get started with that. Looks like POSIX rename does not support cross device renaming and such an operation fails with errno set to EXDEV.
Refer to https://pubs.opengroup.org/onlinepubs/009604599/functions/rename.html or https://man7.org/linux/man-pages/man2/rename.2.html.
The STL currently supports cross device renaming currently using the MOVEFILE_COPY_ALLOWED flag. So do we keep this as a legacy feature or do we remove it. I need to know the maintainers' stance on this.

This is actually tied to whether we can use FileRenameInfo(Ex) for implementing rename. The workaround for fixing the issue with read-only bit may differ quite a bit depending upon whether we are sticking with MoveFileExW or making use of FileRenameInfo(Ex)

SibiSiddharthan · 2021-07-15T19:20:47Z

I would like to add a few more tests cases to rename. They are related to the third point from https://en.cppreference.com/w/cpp/filesystem/rename

Symlinks are not followed: if old_p is a symlink, it is itself renamed, not its target.
If new_p is an existing symlink, it is itself erased, not its target.

I propose 2 test cases for this:

symlink fileASym -> fileA. Rename fileASym to fileASymRenamed. Read and check fileA contents by opening fileASymRenamed.
symlink fileB -> fileA. Rename fileC to fileB. Read and check fileB(should be fileC) contents. Sanity check fileA contents.

NOTE: These tests cases don't depend on the changes introduced in this PR. So should I bundle these test cases with this PR itself or raise a separate one?

fsb4000 · 2021-07-16T09:19:11Z

Instead of

Issue #2053

write

Fixes #2053

in your first comment.
Then your PR will be linked with the issue. And the issue will be closed automatically when the PR will be merged

SibiSiddharthan · 2021-07-16T11:28:18Z

Instead of
Issue #2053
write
Fixes #2053
in your first comment.
Then your PR will be linked with the issue. And the issue will be closed automatically when the PR will be merged

Thanks for the info

stl/src/filesystem.cpp

AlexGuteniev · 2021-07-21T11:09:48Z

stl/src/filesystem.cpp

 [[nodiscard]] __std_win_error __stdcall __std_fs_rename(
    _In_z_ const wchar_t* const _Source, _In_z_ const wchar_t* const _Target) noexcept {
+    __std_win_error _Last_error;
+    constexpr auto _Flags = __std_fs_file_flags::_Backup_semantics | __std_fs_file_flags::_Open_reparse_point;


Does __std_fs_file_flags::_Backup_semantics work under a limited user account?

Not sure, but we need this to rename a directory.

I don't think MoveFileEx uses this flag internally, as it can use NtOpenFile directly.

My concern is about this part of CreateFileW doc:

FILE_FLAG_BACKUP_SEMANTICS
0x02000000
The file is being opened or created for a backup or restore operation. The system ensures that the calling process overrides file security checks when the process has SE_BACKUP_NAME and SE_RESTORE_NAME privileges. For more information, see Changing Privileges in a Token.

If there's a chance that there's dependency on privilege, the function may behave inconsistently, depending on that.

Do we have a way to get a HANDLE to a directory without FILE_FLAG_BACKUP_SEMANTICS? I am not sure about using the Nt* stuff. Is it allowed here?

I don't know. Undocumented stuff is not allowed (#705). But maybe NtOpenFile is fine, there's a documentation https://docs.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntopenfile

Would be good if just using FILE_FLAG_BACKUP_SEMANTICS is fine, but need to confirm it.

FILE_FLAG_BACKUP_SEMANTICS can be used by limited users; we already use it elsewhere in this implementation. The documentation is somewhat confusing, what it means is that if you have taken appropriate privileges, and you use the flag, then you get the effects of those privileges (ignoring read restrictions with SeBackupPrivilege, and write restrictions with SeRestorePrivilege). If you haven't taken those privileges then you still get the ability to open a directory and other effects of the flag, you just don't get to ignore ACLs.

I don't know. Undocumented stuff is not allowed (#705). But maybe NtOpenFile is fine, there's a documentation https://docs.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntopenfile

NtOpenFile is not available under the "app"/"onecore" flavors, to my understanding.

stl/src/filesystem.cpp

AlexGuteniev · 2021-07-21T11:19:38Z

stl/src/filesystem.cpp

+        FILE_ID_INFO Id_a, Id_b;
+        if (GetFileInformationByHandleEx(_Handle_a, FileIdInfo, &Id_a, sizeof(FILE_ID_INFO))
+            && GetFileInformationByHandleEx(_Handle_b, FileIdInfo, &Id_b, sizeof(FILE_ID_INFO))) {
+            return memcmp(Id_a.FileId.Identifier, Id_b.FileId.Identifier, sizeof(FILE_ID_128)) == 0;


A question - how would this behave on simpler filesystems, like FAT or exFAT. They don't have hard links, but do they have FILE_ID_INFOs? And if not, would GetFileInformationByHandleEx fail, or successfully return zero/garbage?

Also looks like VolumeSerialNumber should be compared too, there may be different volumes containing files with the same ID

rename does not work with files across volumes. See https://pubs.opengroup.org/onlinepubs/009604599/functions/rename.html or https://man7.org/linux/man-pages/man2/rename.2.html. So I don't think we should compare VolumeSerialNumber.

With regards to FAT filesystems, I pulled out a USB stick and tried it. Looks like GetFileInformationByHandleEx fails with ERROR_INVALID_PARAMETER when we request FileIdInfo. So no worries 😄

AlexGuteniev · 2021-07-21T11:22:17Z

stl/src/filesystem.cpp

+
+            // Choosing FILE_STANDARD_INFO over FILE_BASIC_INFO to check whether the handles refer to directories.
+            // Because sizeof(FILE_STANDARD_INFO) = 24 < sizeof(FILE_BASIC_INFO) = 40.
+            // How expensive is this operation? (comment for reviewers)


My guess it does not matter much. Though FILE_STANDARD_INFO is more demanded information while working with file, so more likely to be easier to get.

kobykahane · 2021-07-23T17:46:52Z

stl/src/filesystem.cpp

+    [[nodiscard]] bool __stdcall _Is_same_file(_In_ HANDLE _Handle_a, _In_ HANDLE _Handle_b) {
+        constexpr auto _Flags = __std_fs_file_flags::_Backup_semantics | __std_fs_file_flags::_Open_reparse_point;
+        FILE_ID_INFO Id_a, Id_b;
+        if (GetFileInformationByHandleEx(_Handle_a, FileIdInfo, &Id_a, sizeof(FILE_ID_INFO))


Note that FILE_ID_INFO is not available on Windows 7. Consider falling back to GetFileInformationByHandle and examining BY_HANDLE_FILE_INFORMATION::nFileIndexLow/High.

Yes, probably author can reuse:

__std_fs_get_file_id function:

STL/stl/src/filesystem.cpp

Lines 414 to 454 in bd7adb4

_Success_(return == __std_win_error::_Success) __std_win_error

__stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* const _Id, _In_z_ const wchar_t* const _Path) noexcept {

__std_win_error _Last_error;

const _STD _Fs_file _Handle(

_Path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error);

if (_Last_error != __std_win_error::_Success) {

return _Last_error;

}

static_assert(sizeof(FILE_ID_INFO) == sizeof(__std_fs_file_id));

static_assert(alignof(FILE_ID_INFO) == alignof(__std_fs_file_id));

if (GetFileInformationByHandleEx(_Handle._Get(), FileIdInfo, reinterpret_cast<FILE_ID_INFO*>(_Id), sizeof(*_Id))

!= 0) {

// if we could get FILE_ID_INFO, use that as the source of truth

return __std_win_error::_Success;

}

_Last_error = __std_win_error{GetLastError()};

switch (_Last_error) {

case __std_win_error::_Not_supported:

case __std_win_error::_Invalid_parameter:

break; // try more things

default:

return _Last_error; // real error, bail to the caller

}

#ifndef _CRT_APP

// try GetFileInformationByHandle as a fallback

BY_HANDLE_FILE_INFORMATION _Info;

if (GetFileInformationByHandle(_Handle._Get(), &_Info) != 0) {

_Id->_Volume_serial_number = _Info.dwVolumeSerialNumber;

_CSTD memcpy(&_Id->_Id[0], &_Info.nFileIndexHigh, 8);

_CSTD memset(&_Id->_Id[8], 0, 8);

return __std_win_error::_Success;

}

_Last_error = __std_win_error{GetLastError()};

#endif // _CRT_APP

return _Last_error;

}

and struct __std_fs_file_id:

STL/stl/inc/xfilesystem_abi.h

Lines 215 to 218 in ccc5aaa

struct __std_fs_file_id { // typedef struct _FILE_ID_INFO {

unsigned long long _Volume_serial_number; // ULONGLONG VolumeSerialNumber;

unsigned char _Id[16]; // FILE_ID_128 FileId;

}; // } FILE_ID_INFO, ...;

Note that FILE_ID_INFO is not available on Windows 7. Consider falling back to GetFileInformationByHandle and examining BY_HANDLE_FILE_INFORMATION::nFileIndexLow/High.

I have a few questions regarding BY_HANDLE_FILE_INFORMATION::nFileIndexLow/High and FILE_ID_INFO::FileId.
According to the docs both of them represent some sort of File Ids. I did some testing with a bunch of random files across different volumes, I found that BY_HANDLE_FILE_INFORMATION::nFileIndexLow/High , FILE_ID_INFO::FileId.Identifier[0..7] are different and FILE_ID_INFO::FileId.Identifier[8..15] is always 0. BY_HANDLE_FILE_INFORMATION::dwVolumeSerialNumber and FILE_ID_INFO::VolumeSerialNumber are also different.
Can anyone explain me why is this?

For checking whether the files are hardlinks I think it is better to stick to BY_HANDLE_FILE_INFORMATION::nFileIndexLow/High for Windows 7 compatibilty.

Regarding the use of __std_fs_get_file_id, I am still awaiting a response to this #2062 (comment) .

On NTFS file ids are 64-bit values. ReFS has 128-bit file ids. FILE_ID_INFO etc. were introduced to support the expanded file id space needed by ReFS, but on NTFS the ids remain 64-bit values and the upper 64-bits will always be zero. In order to properly support all edge cases on ReFS volumes, it is best to use the 128-bit file id type on systems that support it.

Similarly, the legacy structure has a 32-bit VSN field and the new structure returns the full 64-bit VSN - the different values are due to truncation.

BillyONeal · 2021-07-29T07:07:36Z

Even more so than remove, I believe the most important property of rename is that it is atomic -- it is considerably more important that it be so than remove because rename is the only usually atomic filesystem operation. I think a more appropriate fix would be to document this divergence under http://eel.is/c++draft/fs.conform.9945#2

AlexGuteniev · 2021-07-29T07:26:39Z

I believe the most important property of rename is that it is atomic -- it is considerably more important that it be so than remove because rename is the only usually atomic filesystem operation.

Can it be done atomic by not calling MoveFileExW and renaming via SetFileInformationByHandle+FileRenameInfo and handles reused from collision check?

kobykahane · 2021-07-29T07:31:39Z

I believe the most important property of rename is that it is atomic -- it is considerably more important that it be so than remove because rename is the only usually atomic filesystem operation.

Can it be done atomic by not calling MoveFileExW and renaming via SetFileInformationByHandle+FileRenameInfo and handles reused from collision check?

If the overall goal is POSIX-compliance, I would suggest FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS over plain FileRenameInfo, when available.

AlexGuteniev · 2021-07-29T07:51:09Z

I would suggest FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS over plain FileRenameInfo, when available.

From the documentation:

If FILE_RENAME_REPLACE_IF_EXISTS is also specified, allow replacing a file even if there are existing handles to it. Existing handles to the replaced file continue to be valid for operations such as read and write. Any subsequent opens of the target name will open the renamed file, not the replaced file.

Doesn't seem to address empty directory issue

kobykahane · 2021-07-29T08:19:13Z

I would suggest FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS over plain FileRenameInfo, when available.

From the documentation:

If FILE_RENAME_REPLACE_IF_EXISTS is also specified, allow replacing a file even if there are existing handles to it. Existing handles to the replaced file continue to be valid for operations such as read and write. Any subsequent opens of the target name will open the renamed file, not the replaced file.

Doesn't seem to address empty directory issue

True. FileRenameInfoEx's POSIX semantics would make it immediately overwrite an existing target file name even if it is currently open, but attempts to overwrite an existing empty directory still fail with ERROR_ACCESS_DENIED.

SibiSiddharthan · 2021-07-29T08:43:21Z

If the overall goal is POSIX-compliance, I would suggest FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS over plain FileRenameInfo, when available.

My plan was to use FileRenameInfoEx with FILE_RENAME_REPLACE_IF_EXISTS , FILE_RENAME_POSIX_SEMANTICS and FILE_RENAME_IGNORE_READONLY_ATTRIBUTE.

Windows 7 only supports FileRenameInfo. Windows 10 versions older than 1809 only support FILE_RENAME_IGNORE_READONLY_ATTRIBUTE and versions olders than 1607 support the other flags. So we would need to have appropriate fallbacks for older versions

BillyONeal · 2021-07-29T09:28:53Z

I think “use recent Win10 if you care about these cases” is an acceptable outcome, in the same way that remove can’t remove open things on old OSes.

To the best of my understanding we can’t call any NtXxx things — it isn’t about whether they’re documented, it’s about whether they will be accepted by the Store, and whether they have apisets (to be callable from OneCore environments)

Fixed bugs 1) Renaming files which are hard-links of each other should be a no-op. 2) Renaming directories where the new path points to an empty directory should succeed. Added test cases as well for the above.

Changes: Renamed _Same_file -> _Is_same_file Better return way for _Is_same_file

blazej-czapp · 2022-05-24T17:04:57Z

Just to butt in, we're seeing an issue with std::filesystem::rename() on Windows where it fails to rename an open file (open by an antivirus or some such). Using FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS did seem to help in our case. As it stands at the moment, this PR doesn't seem to address this aspect of POSIX compliance?

SibiSiddharthan · 2022-05-24T17:39:41Z

Just to butt in, we're seeing an issue with std::filesystem::rename() on Windows where it fails to rename an open file (open by an antivirus or some such). Using FileRenameInfoEx with FILE_RENAME_FLAG_POSIX_SEMANTICS did seem to help in our case. As it stands at the moment, this PR doesn't seem to address this aspect of POSIX compliance?

Hey, if you refer to this comment #2062 (comment). I have my proposed changes for rename. This does include FILE_RENAME_FLAG_POSIX_SEMANTICS.

sylveon · 2022-05-24T20:33:51Z

I'm not sure we want POSIX semantics, because it means suddenly this function becomes case sensitive. So you can end up in a situation where you have two files with the same name but different cases after rename, which is a pain for the user to resolve (because almost no tooling is case sensitive so they will behave incorrectly upon trying to delete the file - for example not even fs::remove is case sensitive, afaik)

SibiSiddharthan · 2022-05-25T04:42:16Z

I'm not sure we want POSIX semantics, because it means suddenly this function becomes case sensitive. So you can end up in a situation where you have two files with the same name but different cases after rename, which is a pain for the user to resolve (because almost no tooling is case sensitive so they will behave incorrectly upon trying to delete the file - for example not even fs::remove is case sensitive, afaik)

POSIX semantics does not mean case sensitive. It means that you will have the ability to rename a file even though there are existing handles open to it. See here https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/ns-ntifs-_file_rename_information

blazej-czapp · 2022-05-25T09:47:32Z

Hey, if you refer to this comment #2062 (comment). I have my proposed changes for rename. This does include FILE_RENAME_FLAG_POSIX_SEMANTICS.

Yep, that's the idea. From what I can see, the existing implementation of remove() tries FILE_DISPOSITION_FLAG_POSIX_SEMANTICS and then falls back on other means if the flag isn't supported (i.e. Windows too old) by checking the error code returned by SetFileInformationByHandle(). No idea how reliable that is, but at least there's a precedent :)

BillyONeal · 2022-05-26T20:09:15Z

Yep, that's the idea. From what I can see, the existing implementation of remove() tries FILE_DISPOSITION_FLAG_POSIX_SEMANTICS and then falls back on other means if the flag isn't supported (i.e. Windows too old) by checking the error code returned by SetFileInformationByHandle(). No idea how reliable that is, but at least there's a precedent :)

There is a big difference though in that rename being atomic is important, particularly since it is the only filesystem operating that is reliably atomic everywhere.

SibiSiddharthan · 2022-05-26T22:11:17Z

There is a big difference though in that rename being atomic is important, particularly since it is the only filesystem operating that is reliably atomic everywhere.

Hey, @BillyONeal can you clarify whether rename is atomic in Windows. If so what do we need to do. If not is there any workaround for it.

BillyONeal · 2022-05-26T22:15:25Z

Hey, @BillyONeal can you clarify whether rename is atomic in Windows. If so what do we need to do. If not is there any workaround for it.

It is, but it would not be with this change.

SibiSiddharthan · 2022-05-27T05:45:29Z

Hey, @BillyONeal can you clarify whether rename is atomic in Windows. If so what do we need to do. If not is there any workaround for it.

It is, but it would not be with this change.

Alright, So using SetFileInformationByHandle() with FileRenameInfoEx will be a atomic operation, but checking for hardlinks and then doing rename would be not. Correct me if I am wrong.

BillyONeal · 2022-05-27T14:39:35Z

Alright, So using SetFileInformationByHandle() with FileRenameInfoEx will be a atomic operation, but checking for hardlinks and then doing rename would be not. Correct me if I am wrong.

I believe that's right but I will defer to current maintainers here.

strega-nil-ms · 2022-06-20T22:30:32Z

So first, the current implementation's behavior:

rename(hlink1, hlink2) results in success - basically, hlink1 being deleted, since hlink1 is the same file as hlink2.
rename(nonempty-dir, empty-dir) results in ERROR_ACCESS_DENIED

My opinion:

I am of the same opinion as Billy; making our implementation non-atomic is unacceptable. I'd be willing to take a look at the SetFileInformationByHandle implementation, though - as long as it's atomic. I don't believe any checking of hard links is acceptable here. We are not POSIX, and we should not attempt to be POSIX, unless it can be done without destroying our atomicity guarantees.

Marking as draft as the implementation needs to be changed completely.

@SibiSiddharthan thank you for your work so far, I really appreciate it! sorry for letting this languish for so long :(

SibiSiddharthan · 2022-06-22T20:49:06Z

Hey,

I am of the same opinion as Billy; making our implementation non-atomic is unacceptable.

I understand your decision.

I'd be willing to take a look at the SetFileInformationByHandle implementation, though - as long as it's atomic. I don't believe any checking of hard links is acceptable here. We are not POSIX, and we should not attempt to be POSIX, unless it can be done without destroying our atomicity guarantees.

The SetFileInformationByHandle implementation would use following flags FILE_RENAME_REPLACE_IF_EXISTS, FILE_RENAME_FLAG_POSIX_SEMANTICS and FILE_RENAME_IGNORE_READONLY_ATTRIBUTE.

Since FILE_RENAME_IGNORE_READONLY_ATTRIBUTE is only supported after Windows 10 1809, we would need to have fallbacks for older versions. This implementation would address rename(nonempty-dir, empty-dir) and rename(file,read-only-file).

For older Windows versions as cannot remove the readonly bit and delete the file atomically, we will avoid the FILE_RENAME_IGNORE_READONLY_ATTRIBUTE flag in the fallbacks.

This will also address rename("A:\\file","B:\\file"). The current implementation will move the file from A: to B:, the proposed would fail such a request as it should.(I doubt that copying a file between drives is atomic).

In Summary, newer Windows versions will have a more standard conforming rename compared to older versions.
@strega-nil-ms please let me know your thoughts on this.

strega-nil-ms · 2022-06-23T17:56:57Z

@SibiSiddharthan disallowing rename(R"(A:\file)", R"(B:\file)") is unacceptable; neither POSIX rename nor standard filesystem::rename have anything to say about cross-device renames, so they should work (however, they don't have to be atomic, since obviously that's not possible).

I'm not sure; I'm curious how Billy feels. It makes me uncomfortable, but not overly so. I think I would still prefer to use MoveFileEx if possible, since that's the "obvious" API for rename and I'm a big believer in "filesystem should act like the native API".

SibiSiddharthan · 2022-06-23T18:09:56Z

@SibiSiddharthan disallowing rename(R"(A:\file)", R"(B:\file)") is unacceptable; neither POSIX rename nor standard filesystem::rename have anything to say about cross-device renames.

Yes they do. Please refer this link https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html. See error code EXDEV.

EDIT: I have gone through the standard again. Looks like std::rename does not say anything about cross-device renames. But std::filesystem::rename does specify the behavior for such a request.

BillyONeal · 2022-06-23T21:03:57Z

@SibiSiddharthan disallowing rename(R"(A:\file)", R"(B:\file)") is unacceptable; neither POSIX rename nor standard filesystem::rename have anything to say about cross-device renames, so they should work (however, they don't have to be atomic, since obviously that's not possible).

I'm not sure; I'm curious how Billy feels. It makes me uncomfortable, but not overly so. I think I would still prefer to use MoveFileEx if possible, since that's the "obvious" API for rename and I'm a big believer in "filesystem should act like the native API".

I believe the C++ standard referencing POSIX here is only because POSIX was an ISO standard, and we didn't want to have to define "what is a file" and every semantic over again from there. http://eel.is/c++draft/fs.conform.9945#2 is the explicit callout to be "get as close to that as is reasonable" and I don't think we should be issuing substantially higher numbers of syscalls or similar to replicate every edge case.

That said, in this specific case, MoveFileEx already doesn't allow movement between file systems without MOVEFILE_COPY_ALLOWED so we we can replicate this if we want by just not including that flag.

Whether we can now that std::filesystem has already been shipping for 5 years or so is something else. It isn't a change I would make but I think it would be a "righteous" one if it is indeed clear that POSIX prohibits it.

@SibiSiddharthan disallowing rename(R"(A:\file)", R"(B:\file)") is unacceptable; neither POSIX rename nor standard filesystem::rename have anything to say about cross-device renames.

Yes they do. Please refer this link https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html. See error code EXDEV.

EDIT: I have gone through the standard again. Looks like std::rename does not say anything about cross-device renames. But std::filesystem::rename does specify the behavior for such a request.

Hmmmmm I'm not sure I'm convinced on this one. It says that you might get this error in "you may get these errors" remarks but that doesn't seem like "all implementations must give you this error". As for std::filesystem::rename, it certainly doesn't say anything about that case in http://eel.is/c++draft/fs.op.rename . (And in fact divergences from POSIX are explicitly called out there) Also, POSIX says there that they defer to the C standard. Our ::rename does pass MOVEFILE_COPY_ALLOWED:

strega-nil-ms · 2022-07-07T18:05:09Z

I am marking this decision needed; I do not believe that this PR is something we want to do. POSIX-compliance here, at the expense of atomicity, is not interesting to me. However, I do want to discuss with everyone.

Thank you, @SibiSiddharthan, for this PR! It's still a good PR even if it is not something we wish to take.

SibiSiddharthan · 2022-07-10T04:38:56Z

Hey,
Thank you for the inputs provided. I do find myself in agreement with @BillyONeal's reasoning. Will wait till a decision has been reached.

PS: Sorry for the late reply. Have been quite busy lately.

StephanTLavavej · 2022-07-13T21:29:00Z

Thanks again @SibiSiddharthan - we talked about this at the weekly maintainer meeting and everyone concurs with closing this PR and keeping the current atomic behavior.

SibiSiddharthan requested a review from a team as a code owner July 15, 2021 18:48

SibiSiddharthan mentioned this pull request Jul 15, 2021

<filesystem>: rename is not POSIX compliant #2053

Open

SibiSiddharthan force-pushed the main branch 2 times, most recently from adfab0b to 16873c5 Compare July 15, 2021 19:23

StephanTLavavej added the bug Something isn't working label Jul 16, 2021

AdamBucior reviewed Jul 16, 2021

View reviewed changes

stl/src/filesystem.cpp Outdated Show resolved Hide resolved

stl/src/filesystem.cpp Outdated Show resolved Hide resolved

AlexGuteniev reviewed Jul 21, 2021

View reviewed changes

kobykahane reviewed Jul 23, 2021

View reviewed changes

SibiSiddharthan force-pushed the main branch from f6cce9a to 38fd9ea Compare July 23, 2021 18:43

StephanTLavavej self-assigned this Jul 28, 2021

SibiSiddharthan added 2 commits March 22, 2022 00:13

Make filesystem::rename POSIX compliant

506287e

Fixed bugs 1) Renaming files which are hard-links of each other should be a no-op. 2) Renaming directories where the new path points to an empty directory should succeed. Added test cases as well for the above.

Addressing PR reviews

d2187c7

Changes: Renamed _Same_file -> _Is_same_file Better return way for _Is_same_file

SibiSiddharthan force-pushed the main branch from 38fd9ea to d2187c7 Compare March 21, 2022 18:45

StephanTLavavej added the filesystem C++17 filesystem label May 6, 2022

StephanTLavavej assigned strega-nil-ms and unassigned StephanTLavavej Jun 4, 2022

Merge remote-tracking branch 'upstream/main' into SibiSiddharthan/main

3bf2f69

strega-nil-ms removed their assignment Jun 20, 2022

strega-nil-ms marked this pull request as draft June 20, 2022 22:31

StephanTLavavej assigned strega-nil-ms Jun 29, 2022

strega-nil-ms removed the bug Something isn't working label Jun 30, 2022

strega-nil-ms added the decision needed We need to choose something before working on this label Jul 7, 2022

StephanTLavavej removed the decision needed We need to choose something before working on this label Jul 13, 2022

StephanTLavavej closed this Jul 13, 2022

strega-nil-ms removed their assignment Jul 26, 2022

nhz2 mentioned this pull request Aug 18, 2024

Document and test Base.rename JuliaLang/julia#55503

Merged

	_Success_(return == __std_win_error::_Success) __std_win_error
	__stdcall __std_fs_get_file_id(_Out_ __std_fs_file_id* const _Id, _In_z_ const wchar_t* const _Path) noexcept {
	__std_win_error _Last_error;
	const _STD _Fs_file _Handle(
	_Path, __std_access_rights::_File_read_attributes, __std_fs_file_flags::_Backup_semantics, &_Last_error);
	if (_Last_error != __std_win_error::_Success) {
	return _Last_error;
	}

	static_assert(sizeof(FILE_ID_INFO) == sizeof(__std_fs_file_id));
	static_assert(alignof(FILE_ID_INFO) == alignof(__std_fs_file_id));
	if (GetFileInformationByHandleEx(_Handle._Get(), FileIdInfo, reinterpret_cast<FILE_ID_INFO>(_Id), sizeof(_Id))
	!= 0) {
	// if we could get FILE_ID_INFO, use that as the source of truth
	return __std_win_error::_Success;
	}

	_Last_error = __std_win_error{GetLastError()};
	switch (_Last_error) {
	case __std_win_error::_Not_supported:
	case __std_win_error::_Invalid_parameter:
	break; // try more things
	default:
	return _Last_error; // real error, bail to the caller
	}

	#ifndef _CRT_APP
	// try GetFileInformationByHandle as a fallback
	BY_HANDLE_FILE_INFORMATION _Info;
	if (GetFileInformationByHandle(_Handle._Get(), &_Info) != 0) {
	_Id->_Volume_serial_number = _Info.dwVolumeSerialNumber;
	_CSTD memcpy(&_Id->_Id[0], &_Info.nFileIndexHigh, 8);
	_CSTD memset(&_Id->_Id[8], 0, 8);
	return __std_win_error::_Success;
	}

	_Last_error = __std_win_error{GetLastError()};
	#endif // _CRT_APP

	return _Last_error;
	}

	struct __std_fs_file_id { // typedef struct _FILE_ID_INFO {
	unsigned long long _Volume_serial_number; // ULONGLONG VolumeSerialNumber;
	unsigned char _Id[16]; // FILE_ID_128 FileId;
	}; // } FILE_ID_INFO, ...;

Conversation

SibiSiddharthan commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SibiSiddharthan commented Jul 15, 2021

Uh oh!

SibiSiddharthan commented Jul 15, 2021

Uh oh!

fsb4000 commented Jul 16, 2021

Uh oh!

SibiSiddharthan commented Jul 16, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsb4000 Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Jul 29, 2021

Uh oh!

AlexGuteniev commented Jul 29, 2021

Uh oh!

kobykahane commented Jul 29, 2021

Uh oh!

AlexGuteniev commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kobykahane commented Jul 29, 2021

Uh oh!

SibiSiddharthan commented Jul 29, 2021

Uh oh!

BillyONeal commented Jul 29, 2021

Uh oh!

blazej-czapp commented May 24, 2022

Uh oh!

SibiSiddharthan commented May 24, 2022

Uh oh!

sylveon commented May 24, 2022

Uh oh!

SibiSiddharthan commented May 25, 2022

Uh oh!

blazej-czapp commented May 25, 2022

Uh oh!

BillyONeal commented May 26, 2022

Uh oh!

SibiSiddharthan commented May 26, 2022

Uh oh!

BillyONeal commented May 26, 2022

Uh oh!

SibiSiddharthan commented May 27, 2022

Uh oh!

BillyONeal commented May 27, 2022

Uh oh!

SibiSiddharthan commented Jul 15, 2021 •

edited

Loading

fsb4000 Jul 23, 2021 •

edited

Loading

AlexGuteniev commented Jul 29, 2021 •

edited

Loading

strega-nil-ms commented Jun 20, 2022 •

edited

Loading

SibiSiddharthan commented Jun 22, 2022 •

edited

Loading

SibiSiddharthan commented Jun 23, 2022 •

edited

Loading