This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author eryksun
Recipients bup, eryksun
Date 2022-02-19.12:05:17
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1645272317.95.0.416835249315.issue46791@roundup.psfhosted.org>
In-reply-to
Content
In Windows, checking for a directory in order to defer to either os_rmdir_impl() or os_unlink_impl() would lead to a redundant directory check. os_unlink_impl() already has to check for a directory in order to delete a directory symlink or mountpoint. I suggest implementing a common _Py_remove() function in Windows. For example:

    typedef enum {
        REMOVE_FILE,
        REMOVE_DIR,
        REMOVE_BOTH,
    } _Py_remove_mode;

    int
    _Py_remove(path_t *path, _Py_remove_mode mode)
    {
        BOOL isDir = FALSE;
        BOOL isLnk = FALSE;
        BOOL success = FALSE;

        if (mode != REMOVE_DIR) {
            DWORD fileAttributes = GetFileAttributesW(path->wide);
            if (fileAttributes != INVALID_FILE_ATTRIBUTES) {
                isDir = fileAttributes & FILE_ATTRIBUTE_DIRECTORY;
            }

            if (isDir && (mode == REMOVE_FILE)) {
                WIN32_FIND_DATAW data;
                HANDLE hFind = FindFirstFileW(path->wide, &data);
                if (hFind != INVALID_HANDLE_VALUE) {
                    FindClose(hFind);
                    if (data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) {
                        isLnk = IsReparseTagNameSurrogate(data.dwReserved0);
                    }
                }
            }
        }

        if (mode == REMOVE_DIR || isDir && (mode == REMOVE_BOTH || isLnk)) {
            success = RemoveDirectoryW(path->wide);
        } else {
            success = DeleteFileW(path->wide);
        }
        
        return success ? 0 : -1;
    }

The multiple opens for GetFileAttributesW(), FindFirstFileW() and DeleteFileW() add up to make the delete more expensive and more race prone than it has to be. It would be nice to use a single open for all operations. But the Windows API just has CreateFileW(), which requires SYNCHRONIZE access. The latter can't be granted by the parent directory, unlike FILE_READ_ATTRIBUTES and DELETE access. We could implement a version that uses CreateFileW(), and fall back on the above version when access is denied, similar to what we do for os.stat(). Also, Python is limited to the Windows 8.1 SDK, which makes it awkward to use FileDispositionInfoEx (POSIX delete) like DeleteFileW() does in Windows 10, but it should still be possible.
History
Date User Action Args
2022-02-19 12:05:17eryksunsetrecipients: + eryksun, bup
2022-02-19 12:05:17eryksunsetmessageid: <1645272317.95.0.416835249315.issue46791@roundup.psfhosted.org>
2022-02-19 12:05:17eryksunlinkissue46791 messages
2022-02-19 12:05:17eryksuncreate