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

file/open: check if directory #1532

Merged
merged 2 commits into from
Dec 14, 2024

Conversation

strangepete
Copy link
Contributor

Adds fstat directory test after fopen, which can return non-NULL when passed a directory name on Linux.

Tested on Ubuntu 22.04 (+valgrind), & Windows 10

Should close #1530

Adds fstat() directory test after fopen(), which can return non-NULL when passed a directory name on Linux
@strangepete
Copy link
Contributor Author

As an afterthought, it's not great that this introduces a different message for the same error on multiple platforms (ie, Windows says 'could not find file')

@jakobhellermann
Copy link

jakobhellermann commented Dec 14, 2024

I can confirm that this does improve the error message on my machine as well

error: cannot open directory: build
  in file/open [src/core/io.c] on line 153
  in dofile [boot.janet] on line 3002, column 12
  in cli-main [boot.janet] on line 4645, column 17

Should use fileno() instead of direct
@bakpakin
Copy link
Member

While I like the error message, something bothers me about making an extra system call every time we open a file. That said, python does the same thing, and there is always os/open for less abstracted IO.

@bakpakin bakpakin merged commit a85eaca into janet-lang:master Dec 14, 2024
13 checks passed
@strangepete
Copy link
Contributor Author

strangepete commented Dec 14, 2024

Being a system call didn't occur to me, but I agree, though looking at the field it seems everyone is fine doing this, which is crazy to me...open/fopen give multiple options to ensure a target is definitely a directory; I'm very curious what the use case is to 'read' a directory in binary...everything I've read suggests nobody actually does that.

Alternatively, as Jakob originally suggested just save the file name for use later if an error pops up. I assume nobody can open enough files to make the memory usage matter?

@strangepete strangepete deleted the fstat-directory-test branch December 15, 2024 16:07
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.

include filename in error: could not read file message
3 participants