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

introduce new function ReadFileNoStat #219

Merged
merged 2 commits into from Oct 4, 2019

Conversation

@pgier
Copy link
Collaborator

commented Oct 2, 2019

This function should be a slight improvement over
ioutil.ReadFile because it skips the call to os.Stat which commonly
returns the wrong file size for files in /proc and /sys.

Usage of this function also makes the code a bit more concise for
parsing of several of the files in /proc.

This function should be a slight improvement over
ioutil.ReadFile because it skips the call to os.Stat which commonly
returns the wrong file size for files in /proc and /sys.

Usage of this function also makes the code a bit more concise for
parsing of several of the files in /proc.

Signed-off-by: Paul Gier <pgier@redhat.com>
@pgier

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 2, 2019

To provide a bit more context on this, I started updating procfs functions to follow the new guidelines to read an entire file at once using ioutil.ReadFile. However, I found several places in the root package where we were basically doing the same thing as ReadFile but without the call to os.Stat. Since stat does not return valid file sizes for most of the files in /proc and /sys it seemed to make sense to skip that part of ioutil.ReadFile.

@pgier pgier requested a review from mdlayher Oct 2, 2019
@pgier pgier requested a review from SuperQ Oct 2, 2019
@pgier pgier unassigned SuperQ Oct 2, 2019
Copy link
Member

left a comment

Seems fine to me. Were the stat calls causing some sort of problem?

@pgier

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 4, 2019

Before the changes we also weren't making calls to stat in these cases, so I was a bit worried that using ioutil.ReadFile could introduce an issue if any of the stat calls were to fail. Separately, I'll make some additional changes to remove usage of ioutil.ReadFile where it's currently being used.

Also, it's a bit tricky to test this well because of course our fixtures directory is a real filesystem instead of virtual.

internal/util/sysreadfile.go Outdated Show resolved Hide resolved
Files larger than this should use a scanner.

Signed-off-by: Paul Gier <pgier@redhat.com>
@pgier pgier merged commit c0c2a8b into prometheus:master Oct 4, 2019
5 checks passed
5 checks passed
ci/circleci: codespell Your tests passed on CircleCI!
Details
ci/circleci: linux-1-11 Your tests passed on CircleCI!
Details
ci/circleci: linux-1-12 Your tests passed on CircleCI!
Details
ci/circleci: windows-1-11 Your tests passed on CircleCI!
Details
ci/circleci: windows-1-12 Your tests passed on CircleCI!
Details

reader := io.LimitReader(f, maxBufferSize)
return ioutil.ReadAll(reader)
}

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 6, 2019

This .go file has the following build constraint on line 14:

// +build linux,!appengine

If you're adding a new function here, you should also add it to internal/util/sysreadfile_compat.go file. Otherwise, this function will only be available on non-appengine linux.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Oct 6, 2019

Unlike SysReadFile, ReadFileNoStat doesn't actually use syscalls, so it should probably be added to another .go file without the build constraint. That way you wouldn't have to have 2 identical implementations of it in sysreadfile.go and sysreadfile_compat.go.

@dmitshur dmitshur referenced this pull request Oct 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.