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 posix delete on windows, if supported #4318

Open
wants to merge 9 commits into
base: v1.x
Choose a base branch
from

Conversation

IanButterworth
Copy link

@IanButterworth IanButterworth commented Feb 19, 2024

Fixes #3839

Implements posix delete for files and dirs, with fallback to the old method if not supported (e.g. Fat32)

@IanButterworth IanButterworth force-pushed the ib/win_posix_delete branch 6 times, most recently from 90d014f to 9e5bbac Compare February 19, 2024 02:16
@IanButterworth IanButterworth marked this pull request as ready for review February 19, 2024 02:31
src/win/fs.c Outdated Show resolved Hide resolved
@vtjnash
Copy link
Member

vtjnash commented Feb 19, 2024

LGTM, but will leave open for a bit in case anyone else wants to comment

@IanButterworth
Copy link
Author

Of the two tests that are failing, I don't see a relation to this PR and:

This one is failing on the tip of v1.x #4325

not ok 32 - env_vars
# exit code -1073740791
# Output from process `env_vars`:
# Assertion failed in D:\a\libuv\libuv\test\test-env-vars.c on line 96: `r == 0` (-4071 == 0)

I take it this one might be flaky?

2: not ok 53 - fs_event_watch_dir
2: # exit code -1073740791
2: # Output from process `fs_event_watch_dir`:
2: # Assertion failed in D:\a\libuv\libuv\test\test-fs-event.c on line 201: `strncmp(filename, file_prefix, sizeof(file_prefix) - 1) == 0` (1 == 0)

src/win/fs.c Outdated Show resolved Hide resolved
src/win/fs.c Outdated Show resolved Hide resolved
src/win/fs.c Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Only a partial review due to time constraints.

src/win/fs.c Show resolved Hide resolved
src/win/fs.c Outdated Show resolved Hide resolved
src/win/fs.c Outdated Show resolved Hide resolved
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM insofar I'm able to judge that.

I note there's a TOCTOU issue in that function (checking file type before applying the operation) but that doesn't look like it's new.

src/win/fs.c Outdated Show resolved Hide resolved
@IanButterworth
Copy link
Author

Gentle bump on this.

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.

provide posix semantics for uv_fs_unlink on Windows
3 participants