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

Export stat! and add it to the manual #50300

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

dalum
Copy link
Contributor

@dalum dalum commented Jun 27, 2023

This is a follow-up to #46410, which was merged before these new symbols were exported and added to the manual. This PR does that.

@jakobnissen
Copy link
Contributor

I know stat! is implicitly tested by testing stat, but perhaps add a test of what happens when e.g. a user passes a vector of the wrong size, or wrong element type?

@dalum
Copy link
Contributor Author

dalum commented Jun 27, 2023

The methods are concretely typed on Vector{UInt8}, so passing a vector of the wrong element type is just a method error. Because of the ccalls, a vector that's too small may cause a segfault, so that's perhaps not the best thing to put in a test. We can add a test that a too large vector allocates enough memory, but that seems superfluous. But perhaps a warning can be added to the docstring, that undefined behaviour, including Julia crashing, can result from calling stat! with a buffer that wasn't created using Base.Filesystem.get_stat_buf()?

@jakobnissen
Copy link
Contributor

jakobnissen commented Jun 27, 2023

Perhaps it would be better to include something like

length(buffer) < STAT_BUFFER_SIZE && resize!(buffer, STAT_BUFFER_SIZE)

, if the size of the statbuffer is a compile time constant. It looks from src/sys.c like it is a compile time constant, so perhaps it would be better to have some Julia level code like

const STAT_BUFFER_SIZE = Int(ccall(:jl_sizeof_stat, Int32, ()))

and then have the conditional resize!. This would prevent exposing segfaulty code to users, while also skipping the ccall to determine the buffer size.

@dalum
Copy link
Contributor Author

dalum commented Jun 27, 2023

That sounds very reasonable, let me add it 😊

@rfourquet
Copy link
Member

Then I would consider also not exposing get_buffer_stat, as users can just pass buf = UInt8[] and buf will be resized to the correct size after the first call to stat!, and instead possibly exposing Base.Filesystem.STAT_BUFFER_SIZE to allow getting the correct size at buffer creation time (with zeros(UInt8, STAT_BUFFER_SIZE)).

@dalum dalum changed the title Export stat! and Base.Filesystem.get_stat_buf and add to manual Export stat! add to manual Jun 27, 2023
@dalum dalum changed the title Export stat! add to manual Export stat! and add it to the manual Jun 27, 2023
@dalum
Copy link
Contributor Author

dalum commented Jun 30, 2023

Since #50323 was merged, I updated this PR to include the reverted changes.

@brenhinkeller brenhinkeller added filesystem Underlying file system and functions that use it merge me PR is reviewed. Merge when all tests are passing labels Aug 6, 2023
@KristofferC
Copy link
Member

Just want to reiterate the comment #46410 (comment).

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filesystem Underlying file system and functions that use it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants