Skip to content

Take O_PATH from x/sys rather than hardcoding it - #61

Open
shalseth wants to merge 1 commit into
containerd:mainfrom
shalseth:sparc64-opath
Open

shalseth wants to merge 1 commit into
containerd:mainfrom
shalseth:sparc64-opath

Conversation

@shalseth

Copy link
Copy Markdown
Contributor

handle_linux.go carried its own copy of the constant:

const O_PATH = 0o10000000

That is right on most Linux architectures but not all. On SPARC, O_PATH is
0x1000000, and 0x200000 - the value above - is O_NOATIME. getHandle
therefore opened the FIFO for reading instead of as a path reference, and
blocked until a writer appeared.

The visible effect under Docker on linux/sparc64 was that every container
hung on start: it reached Created, no shim was ever spawned, and nothing
appeared in any log.

Take the value from golang.org/x/sys/unix, which defines it per
architecture. The standard library cannot be used here - syscall.O_PATH is
undefined on linux/amd64 and linux/386, which is presumably why the constant
was written out by hand in the first place. x/sys is already a direct
dependency and is already imported by fifo.go.

The constant stays exported, so this is not an API change. x/sys gives
0x200000 on every architecture except SPARC, so nothing changes anywhere the
old literal was correct.

@shalseth

Copy link
Copy Markdown
Contributor Author

The failing Project Checks looks unrelated to this change.

Tests and linters pass on every platform. Happy to rebase or force-push if that helps re-trigger anything once the action is sorted.

@shalseth

Copy link
Copy Markdown
Contributor Author

@thaJeztah @samuelkarp, apologies for the ping, but this needs a maintainer's eye.

project-checks@v1.1.0 installs github.com/kunalkushwaha/ltag, which has since moved to containerd/ltag,

The build failure is addressed in #62.

If #62 goes in first, this PR will need its checks re-run to pick up the new workflow.

handle_linux.go carried its own copy of the constant:

	const O_PATH = 0o10000000

That is right on most Linux architectures but not all. On SPARC, O_PATH is
0x1000000, and 0x200000 - the value above - is O_NOATIME. getHandle
therefore opened the FIFO for reading instead of as a path reference, and
blocked until a writer appeared.

The visible effect under Docker on linux/sparc64 was that every container
hung on start: it reached Created, no shim was ever spawned, and nothing
appeared in any log.

Take the value from golang.org/x/sys/unix, which defines it per
architecture. The standard library cannot be used here - syscall.O_PATH is
undefined on linux/amd64 and linux/386, which is presumably why the constant
was written out by hand in the first place. x/sys is already a direct
dependency and is already imported by fifo.go.

The constant stays exported, so this is not an API change. x/sys gives
0x200000 on every architecture except SPARC, so nothing changes anywhere the
old literal was correct.

Signed-off-by: Stian Halseth <stian@itx.no>
@shalseth

Copy link
Copy Markdown
Contributor Author

Rebased after #62 merged. Checks now pass.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Updates Linux handle code to use the canonical unix.O_PATH constant instead of a hard-coded numeric value.

Changes:

  • Add golang.org/x/sys/unix import on Linux.
  • Replace O_PATH constant literal with unix.O_PATH.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread handle_linux.go
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.

2 participants