Skip to content

file/open: check if directory#1532

Merged
bakpakin merged 2 commits intojanet-lang:masterfrom
strangepete:fstat-directory-test
Dec 14, 2024
Merged

file/open: check if directory#1532
bakpakin merged 2 commits intojanet-lang:masterfrom
strangepete:fstat-directory-test

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
@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