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

pkg/system: add mimicked xattr syscalls on FreeBSD #2177

Merged
merged 3 commits into from
Nov 29, 2024
Merged

pkg/system: add mimicked xattr syscalls on FreeBSD #2177

merged 3 commits into from
Nov 29, 2024

Conversation

kimminss0
Copy link
Contributor

@kimminss0 kimminss0 commented Nov 20, 2024 •
edited
Loading

This PR adds mimicked xattr system calls on FreeBSD using extattr. Previous PR #2170 enabled that compatibility on FreeBSD, but only for a very specific area.

EDIT: Sync with the revised changes

@rhatdan
Copy link
Member

rhatdan commented Nov 20, 2024

/approve
LGTM
@nalind @mtrmac @giuseppe @saschagrunert @flouthoc PTAL

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimminss0, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Nov 20, 2024

@dfr PTAL

@dfr
Copy link
Contributor

dfr commented Nov 21, 2024

LGTM

@nalind
Copy link
Member

nalind commented Nov 21, 2024

Why is this new package wrapping the functions with the same names as ones in pkg/system on darwin and linux, instead of adding the freebsd implementation directly to that package? It's not clear to me what benefit we're gaining from the added layer of indirection here.

@kimminss0
Copy link
Contributor Author

The original intention was to let pkg/system be only a set of trivial wrappers of system calls; xattr implementation using extattr system call just seemed non-trivial to me. But I agree that introducing this additional layer might be unnecessary. I'll merge them into pkg/system.

mtrmac
mtrmac reviewed Nov 21, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

I like the general direction of this work.

drivers/chown_unix.go Outdated Show resolved Hide resolved
@kimminss0 kimminss0 force-pushed the xattr-compatibility branch 2 times, most recently from e4d6567 to fabd9f4 Compare November 22, 2024 05:57
@kimminss0 kimminss0 changed the title Add xattr compatibility module for other platforms pkg/system: add mimicked xattr syscalls on FreeBSD Nov 22, 2024
@rhatdan
Copy link
Member

rhatdan commented Nov 22, 2024

@giuseppe @nalind @mtrmac PTAL

@kimminss0
Copy link
Contributor Author

@mtrmac Ping

@kimminss0
pkg/system: remove unreachable error handling
cad02a9 
`extattr_list_link` never returns ENOTATTR, so remove the corresponding
error handling logic to simplify the code.

Signed-off-by: Minseo Kim <[email protected]>
@kimminss0
pkg/system: add xattr syscalls on FreeBSD
be173a9 
FreeBSD does not offer xattr system calls, but it can be mimicked using
extattr system calls.

Signed-off-by: Minseo Kim <[email protected]>
@kimminss0
pkg/system: fix EOPNOTSUPP to ENOTSUP for xattr syscalls
5015968 
The lgetxattr(2), lsetxattr(2), and llistxattr(2) syscalls on Linux
return ENOTSUP instead of EOPNOTSUPP. The same applies to getxattr(2),
setxattr(2), and listxattr(2) on macOS.

Note that EOPNOTSUPP and ENOTSUP have the same value in Linux (refer to
errno(3)).

Signed-off-by: Minseo Kim <[email protected]>
mtrmac
mtrmac reviewed Nov 29, 2024
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@openshift-ci openshift-ci bot added the lgtm label Nov 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 3021f6a into containers:main Nov 29, 2024
20 checks passed
@kimminss0 kimminss0 deleted the xattr-compatibility branch November 29, 2024 15:51
@dfr dfr mentioned this pull request Jan 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees

@mtrmac mtrmac

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kimminss0 @rhatdan @dfr @nalind @mtrmac