Skip to content

Add O_NONBLOCK to the list of supported flags for syscall_fcntl#20745

Open
andrewsomahony wants to merge 6 commits intoemscripten-core:mainfrom
andrewsomahony:fcntl_nonblock
Open

Add O_NONBLOCK to the list of supported flags for syscall_fcntl#20745
andrewsomahony wants to merge 6 commits intoemscripten-core:mainfrom
andrewsomahony:fcntl_nonblock

Conversation

@andrewsomahony
Copy link

We are running in nonblocking mode as it is with Emscripten (blocking + JS is very challenging, to say the least), so any operations that request it should succeed. I've added the flag to fcntl to make it succeed, as I was working with libusb, which uses fcntl and non-blocking IO, and it is now functional with libusb.

// so we allow it
flags = flags & ~(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL |
O_NOCTTY | O_TRUNC);
O_NOCTTY | O_TRUNC | O_NONBLOCK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than ignoring O_NONBLOCK wouldn't we want to pass it through to setFlags? i.e. shouldn't we actually support it?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Sam,

From what I have read, I think it's supported essentially by default, or rather, blocking is not supported.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is true, IIUC all our filesystem support is blocking, which is achieved either by preloading the files into memory or using a background thread to perform async operations.

IIUC we don't currently support O_NONBLOCK, although it would probably very logical to do so.

@tlively ?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Sam,

Ah, yes, you are correct there. So the question then is, in this case, when we call fcntl, what is the use case? My case specifically is with libusb and piping, which from what I understand has to be nonblocking throughout with Emscripten.

-Andrew

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the flag needs to go through the backend, not be ignored here. If the FS supports NON_BLOCKING (as in pipes), and we should accept the flag. For filesystems that don't currently support NON_BLOCKING we should probably reject it.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Sam,

I agree. I'm working on that piece now; I need to get up to speed a little bit with the code, but I'll modify this PR when I have a fix.

Thanks for your help and recommendation!

-Andrew

Copy link
Author

Choose a reason for hiding this comment

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

@sbc100 Hi Sam, I've made some changes to the backend and passed the flag into the backend. Just bumping.

-Andrew

Copy link
Member

Choose a reason for hiding this comment

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

Emscripten pipes are weird because they are non-blocking by default whereas native pipes are blocking by default. I agree we should do something reasonable with O_NONBLOCK and we should let backends support it in other cases as well.

Copy link
Author

Choose a reason for hiding this comment

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

@tlively @sbc100 My change adds the ability for specific file types to block specific flags, such as O_NONBLOCK. Right now, it always returns true for everything, but could you go into more detail for me? What I see with the "Pipe" file type is that it simply writes the input to the output buffer, the one that is read by the other end. I had a gander through the other file types that we currently support and didn't notice any abnormalities.

The behaviour is symmetrical with what we had before, at least with O_APPEND, as there is nowhere that it is checked to see if it can actually be done. Is this correct?

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