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.

Sign up for GitHub

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

Jump to bottom

settings: Only call MkdirAll on save #542

Merged
merged 3 commits into from
Jun 4, 2020
Merged

settings: Only call MkdirAll on save #542

merged 3 commits into from
Jun 4, 2020

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Jun 2, 2020

Previously settingsFileName() created the ~/.config/pprof directory
if it did not exist. However, this is not possible for user nobody,
since its home directory is set to "/nonexistent". Instead, only
create the directory when we actually attempt to save the file, so
the error will happen at the appropriate interaction. This als
prevents pprof from creating empty settings directories.

Fixes the following error when running pprof's web UI as user nobody:

mkdir /nonexistent: permission denied

@evanj
settings: Only call MkdirAll on save
b2b3f5a 
Previously settingsFileName() created the ~/.config/pprof directory
if it did not exist. However, this is not possible for user nobody,
since its home directory is set to "/nonexistent". Instead, only
create the directory when we actually attempt to save the file, so
the error will happen at the appropriate interaction. This als
prevents pprof from creating empty settings directories.

Fixes the following error when running pprof's web UI as user nobody:

    mkdir /nonexistent: permission denied
aalexand
aalexand reviewed Jun 2, 2020
View reviewed changes
internal/driver/settings.go Outdated

// create the settings directory if it does not exist
fdir := filepath.Dir(fname)
if _, err := os.Stat(fdir); os.IsNotExist(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this check needed? I.e. why not os.MkdirAll(fdir, 0700)?

Copy link
Contributor Author

@evanj evanj Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Great suggestion. Mostly because I didn't think of it. It turns out: MkdirAll does exactly this stat right at the beginning. Oops! Removing this now.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2020 •
edited
Loading

Codecov Report

Merging #542 into master will decrease coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
- Coverage   68.33%   68.33%   -0.01%     
==========================================
  Files          39       39              
  Lines        8006     8005       -1     
==========================================
- Hits         5471     5470       -1     
  Misses       2114     2114              
  Partials      421      421
Impacted Files Coverage Δ
internal/driver/settings.go 72.22% <33.33%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 427632f...a46ede1. Read the comment docs.

@aalexand aalexand merged commit 163a225 into google:master Jun 4, 2020
aalexand
aalexand reviewed Jun 4, 2020
View reviewed changes
internal/driver/settings.go
if err := os.MkdirAll(filepath.Dir(fname), 0700); err != nil {
return fmt.Errorf("failed to create settings directory: %w", err)
}

if err := ioutil.WriteFile(fname, data, 0644); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should have been changed to 0600 too.

NHDaly added a commit to NHDaly/Yggdrasil that referenced this pull request Nov 13, 2020
@NHDaly
Update pprof to latest revision
692e645 
Bump from 20191205061153 => 20201109224723

My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`)

Includes:
- google/pprof#564
- google/pprof#575
- google/pprof#574
- google/pprof#571
- google/pprof#572
- google/pprof#570
- google/pprof#562
- google/pprof#561
- google/pprof#565
- google/pprof#560
- google/pprof#563
- google/pprof#557
- google/pprof#554
- google/pprof#552
- google/pprof#545
- google/pprof#549
- google/pprof#547
- google/pprof#541
- google/pprof#534
- google/pprof#542
- google/pprof#535
- google/pprof#531
- google/pprof#530
- google/pprof#528
- google/pprof#522
- google/pprof#525
- google/pprof#527
- google/pprof#519
- google/pprof#520
- google/pprof#517
- google/pprof#518
- google/pprof#514
- google/pprof#513
- google/pprof#510
- google/pprof#508
- google/pprof#506
- google/pprof#509
- google/pprof#504
@NHDaly NHDaly mentioned this pull request Nov 13, 2020
Merged
giordano added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Nov 13, 2020
@NHDaly @giordano
Update pprof to latest revision (#2096)
0784405 
* Update pprof to latest revision

Bump from 20191205061153 => 20201109224723

My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`)

Includes:
- google/pprof#564
- google/pprof#575
- google/pprof#574
- google/pprof#571
- google/pprof#572
- google/pprof#570
- google/pprof#562
- google/pprof#561
- google/pprof#565
- google/pprof#560
- google/pprof#563
- google/pprof#557
- google/pprof#554
- google/pprof#552
- google/pprof#545
- google/pprof#549
- google/pprof#547
- google/pprof#541
- google/pprof#534
- google/pprof#542
- google/pprof#535
- google/pprof#531
- google/pprof#530
- google/pprof#528
- google/pprof#522
- google/pprof#525
- google/pprof#527
- google/pprof#519
- google/pprof#520
- google/pprof#517
- google/pprof#518
- google/pprof#514
- google/pprof#513
- google/pprof#510
- google/pprof#508
- google/pprof#506
- google/pprof#509
- google/pprof#504

* Update P/pprof/build_tarballs.jl - use a real version number

Co-authored-by: Mosè Giordano <[email protected]>

* Remove now unused `timestamp`

* [pprof] Use `GitSource`

Co-authored-by: Mosè Giordano <[email protected]>
gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020
@evanj
settings: Only call MkdirAll on save (google#542)
d4d9798 
settings: Only call MkdirAll on save

Previously settingsFileName() created the ~/.config/pprof directory
if it did not exist. However, this is not possible for user nobody,
since its home directory is set to "/nonexistent". Instead, only
create the directory when we actually attempt to save the file, so
the error will happen at the appropriate interaction. This als
prevents pprof from creating empty settings directories.

Fixes the following error when running pprof's web UI as user nobody:

    mkdir /nonexistent: permission denied
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalexand aalexand aalexand approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanj @codecov-commenter @aalexand @googlebot