GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-99726: Adds os.statx function and associated constants by zooba · Pull Request #99755 · python/cpython

Conversation

@zooba

@zooba zooba commented Nov 24, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Add fast path to os.[l]stat() that returns incomplete information #99726

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
zooba added 6 commits November 24, 2022 15:36
@zooba
pythongh-99726: Adds os.statx function and associated constants
6f7e9fa
@zooba
Update configure
21b5f50
@zooba
Fix POSIX build issues
8fd982e
@zooba
Revert unrelated pyconfig.h.in changes
2b85372
@zooba
Revert generated files
a813f84
@zooba
Update pyconfig.h.in
5a6ce8e
@bedevere-bot bedevere-bot mentioned this pull request Nov 24, 2022
Add fast path to os.[l]stat() that returns incomplete information #99726
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 24, 2022
zooba added 3 commits November 24, 2022 17:00
@zooba
Fixup configure
6f54522
@zooba
Fix docs
cda6886
@zooba
Check for STATX_MNT_ID
0db9341
@zooba zooba mentioned this pull request Nov 24, 2022
gh-99726: Add 'fast' argument to os.[l]stat for faster calculation #99727
Closed
zooba added 2 commits November 24, 2022 17:28
@zooba
Properly exclude statx
e278102
@zooba
Add missing fields
7844111
clintonroy
clintonroy reviewed Nov 24, 2022
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

zooba added 3 commits November 24, 2022 22:59
@zooba
More field initialization
a787ae7
@zooba
More uninitialised fields
b1182c3
@zooba
endif
cc5a159
eryksun
eryksun reviewed Nov 25, 2022
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

zooba added 4 commits November 25, 2022 14:13
@zooba
Switch to less-confusing conditional attributes. Other fixes
ce24e79
@zooba
Few fixes
4a6e5ee
@zooba
Make init config tests more reliable on Windows builds
7835bde
@zooba
Merge remote-tracking branch 'cpython/main' into pythongh-99726
64111f4
zooba
zooba commented Nov 25, 2022
View reviewed changes
Comment thread Python/fileutils.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@zooba

zooba commented Nov 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

My TODO list:

  • Verify whether DeviceType values are devices or types
  • Finish converting stdlib uses of stat()
  • Reread documentation and make sure it's up to date
  • Maybe test whether a totally separate struct would be simpler enough to justify non-interchangeable results
  • More tests
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba added 2 commits November 28, 2022 17:11
@zooba
Merge remote-tracking branch 'cpython/main' into pythongh-99726
d53c013
@zooba
Update DeviceType check and rest of stdlib
991876d
eryksun
eryksun reviewed Nov 28, 2022
View reviewed changes
Comment thread Python/fileutils.c
Comment on lines +1149 to +1171
case FILE_DEVICE_DISK:
case FILE_DEVICE_DISK_FILE_SYSTEM:
case FILE_DEVICE_VIRTUAL_DISK:
case FILE_DEVICE_DFS:
case FILE_DEVICE_CD_ROM:
case FILE_DEVICE_CD_ROM_FILE_SYSTEM:
case FILE_DEVICE_CONTROLLER:
case FILE_DEVICE_DATALINK:
break;
case FILE_DEVICE_CONSOLE:
case FILE_DEVICE_NULL:
case FILE_DEVICE_KEYBOARD:
case FILE_DEVICE_MODEM:
case FILE_DEVICE_MOUSE:
case FILE_DEVICE_PARALLEL_PORT:
case FILE_DEVICE_PRINTER:
case FILE_DEVICE_SCREEN:
case FILE_DEVICE_SERIAL_PORT:
case FILE_DEVICE_SOUND:
/* \\.\nul */
result->st_mode = (result->st_mode & ~S_IFMT) | _S_IFCHR;
break;
case FILE_DEVICE_NAMED_PIPE:

@eryksun eryksun Nov 28, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my earlier comment, we shouldn't arrive in _Py_stat_basic_info_to_stat() for device types that don't implement a filesystem or aren't mounted by a filesystem. That's because, in my experience, FileBasicInformation (file attributes and timestamps) is only implemented for files and directories in a filesystem.

A filesystem type that mounts a volume will return FILE_DEVICE_DISK (e.g. sample FAT32 source code) or FILE_DEVICE_CD_ROM (e.g. sample CDFS source code). FILE_DEVICE_DFS is the distributed filesystem that combines multiple shares. A RAM disk device can use FILE_DEVICE_VIRTUAL_DISK, like the old RAM disk sample driver did, and the driver could implement a custom filesystem, but nowadays generally FILE_DEVICE_DISK or FILE_DEVICE_CD_ROM is used for a RAM disk (the ImDisk driver implements both).

FILE_DEVICE_DISK_FILE_SYSTEM, FILE_DEVICE_CD_ROM_FILE_SYSTEM, and FILE_DEVICE_NETWORK_FILE_SYSTEM are the device types for the base filesystem device, e.g. "\\?\GlobalRoot\FAT", "\\?\GlobalRoot\UdfsCdRom", or "\\?\UNC". os.stat() reports the first two as block devices (S_IFBLK), but FILE_DEVICE_NETWORK_FILE_SYSTEM isn't handled at all because GetFileType() returns FILE_TYPE_UNKNOWN. I don't think these low-level devices fit into the POSIX regime of S_IFCHR vs S_IFBLK, but they're more like S_IFCHR since reads and writes don't use a fixed block size. It's nearly a moot point since there's no useful reason to care. There isn't much to be done with such devices in user-mode applications.

In principle, the stat info classes could be implemented for the named-pipe filesystem on the FILE_DEVICE_NAMED_PIPE device type, but in my tests NPFS doesn't implement FileStatInformation. I asked whether it's planned to implement stat info classes in NPFS.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@zooba zooba Nov 28, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my earlier comment, we shouldn't arrive in _Py_stat_basic_info_to_stat() for device types that don't implement a filesystem or aren't mounted by a filesystem.

Ah, that's a very good point (which I missed). There were some tests that stopped failing with my change here though, so clearly something ends up in this path.

My switch basically mirrors GetFileType, so it should be the same as our existing code.

Other than leading people the wrong way when they read our code, is there anything wrong with checking all the values? There's already a ton of stuff that doesn't map well into POSIX (all the error codes, for example), and I'd prefer our approximations at least be consistent with themselves, even if they aren't meaningful.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@eryksun eryksun Nov 29, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than leading people the wrong way when they read our code, is there anything wrong with checking all the values?

For the stat info classes, they could decide to specially handle some devices in the I/O manager, or implement a fallback in GetFileInformationByName(). I wouldn't want a device to be reported as a regular file instead of either S_IFCHR or S_IFBLK. It shouldn't be a problem as long as they only support the stat info classes for filesystem files and directories -- not for volume devices, filesystem devices, or controllers. (I don't think FILE_DEVICE_DATALINK is used by anything.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@zooba zooba Nov 29, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the cases that should handle devices, though it does seem that the current builds don't return stat information by name for \\?\GlobalRoot\NTFS at least, and it goes through the old function. That could always change though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@eryksun eryksun Nov 29, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they do end up supporting all devices for the stat info classes, it should be okay as long as they require that FileAttributes is set to FILE_ATTRIBUTE_NORMAL for a file that has no other attributes set. That's already required for FileBasicInformation, according to [MS-FSA]. Devices would be distinguished by having FileAttributes set to 0.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
zooba added 2 commits November 28, 2022 23:12
@zooba
Fix refleak and tests
2d90f4d
@zooba
More test fixes
a6d3386
@zooba
Fix os.statx assumption
ed5f370
@zooba
zooba marked this pull request as ready for review November 29, 2022 00:05
@zooba
zooba requested review from a team, brettcannon, jaraco, vsajip and warsaw as code owners November 29, 2022 00:05
zooba
zooba commented Nov 29, 2022
View reviewed changes
Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Doc/library/os.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

zooba and others added 5 commits November 29, 2022 00:21
@zooba
Documentation improvements
4e95220
@zooba
Missed one doc change
3cc69aa
@zooba
Nope, it was applied fine
025a4e6
@zooba
Handle block devices better
8ee94aa
@zooba
Put statx in correct os sets, and include in pythoninfo
bbcc6ee
jaraco
jaraco approved these changes Nov 30, 2022
View reviewed changes

@jaraco jaraco left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, though I haven't analyzed it in detail. If there's any particular critique you're seeking from me, please let me know.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 30, 2022
@zooba

zooba commented Nov 30, 2022

Copy link
Copy Markdown
Member Author

I'm guessing it picked up everyone whose code had a stat in it that I decided to change (a few made more sense to leave alone).

So no specific review requests unless someone has concerns about changing the stat_result struct (I'm basically convinced it's better to modify the existing struct than to create a new one - samestat(statx(), stat()) ought to work).

I'll do a buildbot run, but if anyone has access to a Linux machine with statx and wants to try it out, I'd also appreciate that. My Ubuntu 22 images have it, but don't seem to behave any different.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba zooba added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 30, 2022
@bedevere-bot

bedevere-bot commented Nov 30, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @zooba for commit bbcc6ee 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 30, 2022
@zooba
Merge remote-tracking branch 'cpython/main' into pythongh-99726
768ba42
@brettcannon
brettcannon removed their request for review December 9, 2022 23:38
@zooba

zooba commented Dec 12, 2022

Copy link
Copy Markdown
Member Author

FYI, I've not forgotten about this. I'm waiting to hear back from the Windows team on whether they can fix their new API to provide all the information stat() wants. If they do, I can simply switch to that API directly rather than adding a new one.

That said, if anyone sees enough value in statx generally, I'm happy to have this merged. We might have to tweak the Windows side of things if they change it, but I'm sure they'll find a non-breaking way to make the change.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@carljm
Merge branch 'main' into pythongh-99726
cadeecb
@netlify

netlify Bot commented Dec 15, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit cadeecb
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639b3ffff08a180008b9bd22
😎 Deploy Preview https://deploy-preview-99755--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Dec 18, 2022

Copy link
Copy Markdown
Member

I'm leaving this PR, it looks like it's in good hands.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba zooba mentioned this pull request Mar 6, 2023
gh-99726: Improves correctness of stat results for Windows, and uses faster API when available #102149
Merged
@zooba

zooba commented Mar 16, 2023

Copy link
Copy Markdown
Member Author

Superseded and now massively conflicted, so I'm just going to close this. Adding statx still seems like a good idea if someone wants to do it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@zooba zooba closed this Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eryksun eryksun eryksun left review comments
@jaraco jaraco jaraco approved these changes
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner
+1 more reviewer
@clintonroy clintonroy clintonroy left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@zooba @bedevere-bot @gvanrossum @jaraco @clintonroy @eryksun @carljm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.