Skip to content

Conversation

@thehajime
Copy link
Contributor

fchown(2) on darwin returns EINVAL on anonymous pipe, thus this commit
ignores this checks.

This is related to the PR of containerd (containerd/containerd#4526).

@codecov-commenter
Copy link

Codecov Report

Merging #64 into master will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   12.11%   12.06%   -0.06%     
==========================================
  Files           7        7              
  Lines         652      655       +3     
==========================================
  Hits           79       79              
- Misses        551      554       +3     
  Partials       22       22              
Impacted Files Coverage Δ
io_unix.go 0.00% <0.00%> (ø)

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 23d84c5...71d217e. Read the comment docs.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comment

io_unix.go Outdated
pipes = append(pipes, stdin)
if err = unix.Fchown(int(stdin.r.Fd()), uid, gid); err != nil {
return nil, errors.Wrap(err, "failed to chown stdin")
// XXX: can't chown to pipe on darwin
Copy link
Member

Choose a reason for hiding this comment

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

Suggest using this skip with a warning or debug, and wrap the err: containerd/containerd@55d9550#diff-b3363345815845245ac7ce3ebbab476bR52-R57

for the comment suggest: TODO: revert with proper darwin solution, skipping for now as darwin chown is returning EINVAL on anonymous pipe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated with your suggestion. and also updated go.mod as it now uses logrus package.

io_unix.go Outdated
pipes = append(pipes, stdout)
if err = unix.Fchown(int(stdout.w.Fd()), uid, gid); err != nil {
return nil, errors.Wrap(err, "failed to chown stdout")
// XXX: can't chown to pipe on darwin
Copy link
Member

Choose a reason for hiding this comment

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

..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto.

fchown(2) on darwin returns EINVAL on anonymous pipe, thus this commit
ignores this checks.

Signed-off-by: Hajime Tazaki <[email protected]>
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@AkihiroSuda AkihiroSuda merged commit ad1414d into containerd:master Sep 11, 2020
@thehajime thehajime deleted the fix-darwin-fchown branch September 11, 2020 21:33
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.

4 participants