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

adjust S_IFIFO to avoid collisions #107

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vapier
Copy link

@vapier vapier commented Oct 12, 2019

S_IFIFO currently has the same value as S_IFSOCK which breaks code
that does things like "switch (mode & S_IFMT)" as we end up with
duplicate labels. Move S_IFIFO to a unique unused value.

S_IFIFO currently has the same value as S_IFSOCK which breaks code
that does things like "switch (mode & S_IFMT)" as we end up with
duplicate labels.  Move S_IFIFO to a unique unused value.
@vapier
Copy link
Author

vapier commented Oct 12, 2019

i guess more importantly, the current defines make it impossible to differentiate between fifos & sockets :)

@sbc100 sbc100 requested a review from sunfishcode October 14, 2019 18:50
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

The catch here is that WASI itself doesn't currently have a file type for fifos, so it can't currently distinguish between fifos and sockets. This comes from CloudABI. It was removed here.

At an initial glance, I think it would make sense to add a FIFO filetype to WASI. Even if WASI doesn't support creating new FIFOs right now, and even though FIFOs aren't an entirely portable construct, WASI programs can interact with FIFOs when they do happen to be present. And it can be useful to distinguish between socket and fifo, since you can't openat a socket.

@vapier Would you mind filing a PR to the WASI repo, modifying the typenames.witx file linked above, to add a __WASI_FILETYPE_FIFO? Once we have that, we can add proper libc support.

I guess there's also the question of whether we should land this patch as-is right now. How big of a problem is it if fifos aren't reported as S_IFIFO?

@vapier
Copy link
Author

vapier commented Oct 16, 2019

all of that sounds fine. i'll try to tackle it when i get a chance. thx!

@vapier
Copy link
Author

vapier commented Jan 1, 2020

i posted WebAssembly/WASI#189 to add FIFO filetype

@vapier
Copy link
Author

vapier commented Jan 9, 2020

my WASI PR is merged now. was there something else needed here before merging?

@sunfishcode
Copy link
Member

Before we merge changes into wasi-libc which WASI implementations will need to support, we'll need the WASI Subgroup to publish a new snapshot. There isn't a specific schedule for that yet, but it hopefully won't be too long.

@vapier
Copy link
Author

vapier commented Sep 30, 2020

i think a new snapshot has been posted by now ? so can we merge ?

@sunfishcode
Copy link
Member

Unfortunately no, we have not published a new snapshot yet. Snapshot 2 is set to include modularization, which requires updates to the witx tooling, which is not finished yet. Hopefully once we're past this milestone, subsequent snapshots can be published more frequently.

Base automatically changed from master to main January 19, 2021 23:28
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.

3 participants