Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Windows file metadata, read, and write APIs. #4043

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

CraftSpider
Copy link
Contributor

This also improves how HANDLEs are, well, handled a bit. STDIN/ERR/OUT are now proper pseudo-handles. The invalid handle is added and encode/decodes to exactly 0xFFFFFFFF correctly.

Unix shims require changes to hoist FdTable up, so that Windows can share most of its implementation.

This also improves how HANDLEs are, well, handled a bit. STDIN/ERR/OUT are now proper pseudo-handles. The invalid handle is added and encode/decodes to exactly 0xFFFFFFFF correctly.

Unix shims require changes to hoist FdTable up, so that Windows can share most of its implementation.
@RalfJung
Copy link
Member

Thanks for the PR!

This is a huge PR. It will take a while until I have enough time to take a look at such a big change. If there is any way to stage this as a series of smaller changes, that would be much appreciated.

@CraftSpider
Copy link
Contributor Author

I could possibly further split this down by first PRing the FdMap move, and the handle changes, as their own PRs. In that format, both of those would basically be refactors with no/minimal functional changes, then this PR would just add the shims. It would still be a good bit of stuff, just because even a minimal test needs 4 or 5 shims, but it would be notably smaller hopefully.

@RalfJung
Copy link
Member

That sounds like a good plan. You can keep this PR open (maybe mark it as draft); it can serve as good context for why you want to refactor things the way you suggest.

@RalfJung
Copy link
Member

Also, I know very little about the Windows API... @ChrisDenton @beepster4096 if you have any chance at taking a look at the windows-specific parts here and check whether they look sensible, that would be great. :)

@cgettys-microsoft
Copy link
Contributor

@sivadeilra - here's one of the PRs related to adding Miri support for Windows FS APIs, RE: #3482

@bors
Copy link
Contributor

bors commented Nov 21, 2024

☔ The latest upstream changes (presumably #4046) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines +13 to +15
Stdin,
Stdout,
Stderr,
Copy link
Member

Choose a reason for hiding this comment

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

Note that the handles returned by GetStdHandle are proper file handles and not pseudo-handles. This differs from posix where the fds have fixed values. I realise that miri does want to track the difference but I worry about confusing the terminology.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, some ideas on paths forwards:

  • Leave it as-is, but add documentation about the terminology here
  • Try to merge stdin/out into Handle::File as normal FileDescription (basically what unix does)
    • This will require some more changes to the FileDescription trait. Its read and write methods are a bit tailored towards unix flavor currently, you may notice I have some hacky code in here to work around that. I'd like to fix it eventually anyways, but it adds more friction to MVP
  • Make Handle::File hold an enum like enum FileHandle { Stdin, Stdout, File(i32) }

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Its read and write methods are a bit tailored towards unix flavor currently, you may notice I have some hacky code in here to work around that. I'd like to fix it eventually anyways, but it adds more friction to MVP

Is it even worth using the same trait for Unix and Windows? The trait is indeed very tailored towards Unix, deliberately so. Knowing nothing about the Windows APIs, I expected they would need their own trait anyway.

I haven't looked yet at those hacks. Can you write a high-level summary of the pros and cons of using the same vs a different trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the main pro of reusing the trait at all is because we need a common base trait so long as we don't intend to have two different 'open file maps', one for Windows-like systems and one for Unix-like systems. And most of the FdMap code works well for this purpose - map of numbers to open files is actually a pretty good abstraction for both platforms. The read/write/seek parts of the trait aren't very file-system dependent already, as they're closer to abstractions over the File interface than the syscalls.

To use an entirely different trait, we'd have to somehow support two different dyn Trait in the file code. They also actually would be nearly identical - I think I can remove the main hack I was talking about, looking at it again with fresh eyes.

Copy link
Member

Choose a reason for hiding this comment

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

I expected we'd have two different open file maps, yeah.

But if there's stuff that can be usefully shared between Windows and Unix, even better! I just want to make sure we can keep evolving the Unix side without being held back by constraints or lack of knowledge about what happens on the Windows side.

@RalfJung RalfJung changed the title Implement file metadata, read, and write APIs. Implement Windows file metadata, read, and write APIs. Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants