GeezerVison

A builtin _islink() implementation would open the path using the flags FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS and get the FileAttributeTagInfo. It's a symlink if the query succeeds and ReparseTag is IO_REPARSE_TAG_SYMLINK. Note that IO_REPARSE_TAG_MOUNT_POINT (i.e. a junction) is intentionally excluded from islink().

A few cases that are handled by win32_xstat_impl() should be supported. Perhaps a common function could be implemented that factors out the initial open from win32_xstat_impl() and attributes_from_dir() -- e.g. win32_stat_open(const wchar_t *path, BOOL traverse, WIN32_FIND_DATAW *find_data, BOOL *find_data_updated). Or perhaps simply fall back on win32_xstat_impl() if CreateFileW() fails with one of the error codes discussed below.

• ERROR_ACCESS_DENIED and ERROR_SHARING_VIOLATION: Fall back on using FindFirstFileW(). Internally, this opens the parent directory via NtOpenFile() and queries a directory entry for the name using NtQueryDirectoryFileEx(). The query must fail or return false if the directory entry is a reparse point, unless it's a name-surrogate reparse point (e.g. a symlink or junction) that was supposed to be opened. Here are some cases where this can occur:
  • The file security may grant no access to the caller. Microsoft's filesystems implement a policy that always grants FILE_READ_ATTRIBUTES access if the caller is granted FILE_READ_DATA access to the parent directory. However, CreateFileW() also requests SYNCHRONIZE access.
  • No access is granted to a file that's mapped as a system paging file.
  • No access is granted to a file or directory that has been deleted but is not unlinked, which is the case if there are existing opens and POSIX delete isn't supported by
    • the OS version (e.g. Windows 8),
    • the API function (e.g. RemoveDirectoryW),
    • or the filesystem driver (e.g. exFAT and FAT32).
• ERROR_CANT_ACCESS_FILE: Traversing a reparse point in a path may not be supported because it was created by software on another system (e.g. on a portable drive), or the driver/service that supports the reparse point is no longer installed or running, or it was simply never intended for automatic traversal (e.g. IO_REPARSE_TAG_APPEXECLINK). In this case, the open should be retried with the flag FILE_FLAG_OPEN_REPARSE_POINT. If it turns out to be a name-surrogate reparse point, the query should be failed with the original error code, ERROR_CANT_ACCESS_FILE, as a broken symlink(ish) file.
• ERROR_INVALID_PARAMETER: A device open may require GENERIC_READ access. For example, opening "\\.\CON" (i.e. "\Device\ConDrv\Console") requires either GENERIC_READ or GENERIC_WRITE access. The open has to be retried with the required GENERIC_READ access.

A few cases that are handled by win32_xstat_impl() should be supported.

I guess this means we don't have test coverage for them...?

In any event, I agree we should probably try to cover these cases. I'll see what can be done.

Or perhaps simply fall back on win32_xstat_impl() if CreateFileW() fails with one of the error codes discussed below.

My initial implementation from this morning did that, which I like because it doesn't repeat what's clearly battle-tested code, but it made things slower in the event of symlinks. I was almost ok with that given that that isn't the common case, but your solution seemed to make everything a little faster (ignoring these new corner cases).

Or perhaps simply fall back on win32_xstat_impl() if CreateFileW() fails with one of the error codes discussed below.

My initial implementation from this morning did that, which I like because it doesn't repeat what's clearly battle-tested code, but it made things slower in the event of symlinks.

There shouldn't be an issue with speed for common cases as long as you only fall back on win32_xstat_impl() for the discussed error codes:

• ERROR_ACCESS_DENIED
• ERROR_SHARING_VIOLATION
• ERROR_CANT_ACCESS_FILE
• ERROR_INVALID_PARAMETER

@eryksun: Thanks for all of your help and pointers.

I've implemented support for these corner cases by calling win32_stat under the hood, which is not the most efficient but avoids duplicating the complex logic in win32_xstat_impl.

I'm a little stuck on writing unit tests for them. I was able to cover ERROR_ACCESS_DENIED by extending an existing test for stat to also ensure that isfile matches it.

I'm not sure how to recreate the case you describe in ERROR_CANT_ACCESS_FILE.

For ERROR_INVALID_PARAMETER, I tried isfile(r"\\.\CON") and isdir(r"\\.\CON"), but I get the same result (False) with or without this error handling as well as on main. So maybe I need to somehow create or have CON?

Looks good to me, but I'll add that there's a lot of code duplication in here; it might be worth it to refactor the code shortly after landing this PR, so the added code is more maintainable and readable.

Looks good to me, but I'll add that there's a lot of code duplication in here; it might be worth it to refactor the code shortly after landing this PR, so the added code is more maintainable and readable.

I'll take a first pass at reducing some of this duplication.

My read of the duplication is that there's just enough difference that it's probably not worth it. The main change I'd like to see (but wasn't requiring) is to add an option to path_t for Argument Clinic to let it fall through in the case of errors, rather than raising.

If all the additional functions were using the same OS call, it would make sense to merge them all. But they're subtly different each time, and since this is for perf, we don't want to introduce more switches.

The main change I'd like to see (but wasn't requiring) is to add an option to path_t for Argument Clinic to let it fall through in the case of errors, rather than raising.

+1 (I suggest adding that in a follow-up PR; let's land this.)

If all the additional functions were using the same OS call, it would make sense to merge them all. But they're subtly different each time, and since this is for perf, we don't want to introduce more switches.

True that.

Michael, are you still working on refactoring the code? Steve and Erlend seem to be in agreement that it's ready to merge as is. I think it's ready.

Just heard that it's good to go as it is.

Thanks @mdboom for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

Thanks @mdboom for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Sorry, @mdboom and @zooba, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.10

Sorry @mdboom and @zooba, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.11

This ought to be safe to backport from a public API perspective, but I'll let the bot figure out how easy it'll be.

Thanks @mdboom for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Sorry, @mdboom and @zooba, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.11

Okay, it's not going to backport trivially :) If anyone's up for it, go ahead, otherwise I'll take a look when I get time.

(It's relatively rare that we backport performance improvements, so if it ends up being complicated maybe we shouldn't)