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

Add support for AT_REMOVEDIR in unlinkat syscall #6466

Merged
merged 3 commits into from
Apr 24, 2018

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Apr 20, 2018

It turns out the CPython regression test suite calls unlinkat with the AT_REMOVEDIR flag a lot.
It's in POSIX 2008: http://pubs.opengroup.org/onlinepubs/9699919799.2008edition/

    AT_REMOVEDIR
        Remove the directory entry specified by fd and path as a directory, not a normal file.

    If unlinkat() is passed the special value AT_FDCWD in the fd parameter, the current working directory is used and the behavior shall be identical to a call to unlink() or rmdir() respectively, depending on whether or not the AT_REMOVEDIR bit is set in flag.

@kripken
Copy link
Member

kripken commented Apr 20, 2018

lgtm, thanks! Please just add a test for this. Should probably be in other (we should move more filesystem tests there eventually). See e.g. test_init_file_at_offset there (would be nice to put the test source under tests/fs/ though, which I see that existing test doesn't do).

@mdboom
Copy link
Contributor Author

mdboom commented Apr 20, 2018

Test added.

src = path_from_root('tests', 'fs', 'test_unlink.c')
out = path_from_root('tests', 'fs', 'test_unlink.out')
self.do_run_from_file(src, out, js_engines=[NODE_JS])

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, how we do tests is somewhat confusing. self.emcc_args and do_run_from_file are just for the core test suite.

I opened #6473 now to add a simple helper function for running tests in the other suite, with an example there which is how the test here could be done.

Otherwise, lgtm, thanks!

@mdboom
Copy link
Contributor Author

mdboom commented Apr 24, 2018

Cool. I've updated this to use the new test convention.

@kripken kripken merged commit c8d9b35 into emscripten-core:incoming Apr 24, 2018
@kripken
Copy link
Member

kripken commented Apr 24, 2018

Great, thanks :)

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.

2 participants