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

Error on ambiguous fstflags #242

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Error on ambiguous fstflags #242

merged 2 commits into from
Dec 11, 2023

Conversation

yagehu
Copy link
Contributor

@yagehu yagehu commented Dec 8, 2023

This commit adds a check to the validator macro for fstflags to return inval if the caller requests both atim and atim_now or both mtim and mtim_now because the request is ambiguous. This behavior is consistent across other runtimes (WasmEdge, Wasmtime, WAMR).

This commit adds a check to the validator macro for `fstflags` to return
`inval` if the caller requests both `atim` and `atim_now` or both `mtim`
and `mtim_now` because the request is ambiguous.  This behavior is
consistent across other runtimes (WasmEdge, Wasmtime, WAMR).
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the comprehensive test as well.

src/uvwasi.c Outdated Show resolved Hide resolved
@yagehu
Copy link
Contributor Author

yagehu commented Dec 9, 2023

@guybedford Thanks for the review. Good catch! I fixed all the failing tests.

One notable change is I reordered the validate fstflags call in path_filestat_set_times so that the current badf behavior stays the same regardless of fstflags value.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

LGTM

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 ff7e84f into nodejs:main Dec 11, 2023
7 checks passed
@yagehu yagehu deleted the yagehu/fstflags branch April 10, 2024 17:45
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