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

Add xattr definitions for power/s390x #76

Merged
merged 1 commit into from
May 19, 2017

Conversation

tonistiigi
Copy link
Member

@estesp

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@stevvooe
Copy link
Member

LGTM

@dmcgowan PTAL

@estesp
Copy link
Member

estesp commented May 19, 2017

ping @tophj-ibm ; do you want do a quick look at this? Looks correct to me and is generated from the Go implementations, so I assume correct.

@tophj-ibm
Copy link
Contributor

tophj-ibm commented May 19, 2017

Thanks @estesp and @tonistiigi !!

I just want to reiterate #63 in that the syscall in general isn't the greatest for a lot of reasons and should be switched to /x/sys ASAP. This is especially true for non-x86 architectures where a lot of calls haven't been well tested, and if an issue arrises it often takes an entire release cycle to get in. /rant 😄

Having said that, I don't know of any issues with xattr, and I did check all of these calls against /x/sys just in case and these look fine to me.

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM. This improves support for now with understanding that future state would be to consider x/sys upstream package use per discussion in existing issues.

@dmcgowan
Copy link
Member

LGTM

@dmcgowan dmcgowan merged commit cd7a8e2 into containerd:master May 19, 2017
@justincormack
Copy link
Contributor

@tophj-ibm the only constants here are the actual syscall numbers which are less likely to be wrong. But we could consider other ways of doing this.

@estesp estesp mentioned this pull request Nov 14, 2017
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.

6 participants