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

No-op instead of error on empty read/write #261

Merged
merged 5 commits into from
Apr 22, 2024

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Apr 19, 2024

This commit inserts a special case to fd_{read,write,pread,pwrite} such that passing in an empty list of buffers results in a no-op. This behavior is consistent with Linux host as well as other Wasm runtimes.

fixes #260

This commit inserts a special case to `fd_{read,write,pread,pwrite}`
such that passing in an empty list of buffers results in a no-op.  This
behavior is consistent with Linux host as well as other Wasm runtimes.

fixes nodejs#260
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@yagehu yagehu changed the title No-op instead of error on empty read/write [Draft] No-op instead of error on empty read/write Apr 19, 2024
Since some allocators return NULL if the size is 0, check the length
before preparing buffers so that we don't erroneously return ENOMEM.
@yagehu yagehu changed the title [Draft] No-op instead of error on empty read/write No-op instead of error on empty read/write Apr 19, 2024
@yagehu
Copy link
Contributor Author

yagehu commented Apr 19, 2024

Some allocators return NULL if the size is 0, causing uvwasi__setup_iovs to think allocation has failed. I've moved the len == 0 checks before uvwasi__setup_iovs.

@guybedford @mhdawson Thanks for the review. The PR is ready again.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson merged commit 81ac54a into nodejs:main Apr 22, 2024
7 checks passed
@yagehu yagehu deleted the yagehu/empty-rw branch June 3, 2024 23:46
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.

Behavior diverge from othe runtimes on empty fd_read
3 participants