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

selinux: fix xattr and remove anyhow #2936

Merged
merged 2 commits into from
Sep 29, 2024
Merged

Conversation

Gekko0114
Copy link
Contributor

@Gekko0114 Gekko0114 commented Sep 28, 2024

Fixed several points based on the feedback I received.
#2718

  1. This PR updates the logic of xattr.
    If a file already has an xattr label, the REPLACE method is used.
    If a file doesn't have an xattr label, the CREATE method is used.

  2. This PR removes anyhow from selinux.

Signed-off-by: Hiroyuki Moriya <[email protected]>
@Gekko0114 Gekko0114 changed the title selinux: fix xattr part selinux: fix xattr and remove anyhow Sep 28, 2024
@Gekko0114
Copy link
Contributor Author

Hi @YJDoc2 @utam0k
I've fixed selinux lib based on feedback from @YJDoc2 #2718 (comment)
Could you review it?

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

Hey overall good, a couple of suggestions in the code, with two questions here -

  1. Please run clippy on local and check that all lints are passing, currently that is not getting verified in CI.
  2. I saw that for EINTR, we are returning it as-is to caller. If my understanding is correct EINTR indicates that syscall was interrupted and there might not be any actual error. In which case should we retry? in a loop? I think we had this discussion before somewhere, what was decided from that?

Apart from this, lgtm 👍

experiment/selinux/src/tools/xattr.rs Outdated Show resolved Hide resolved
experiment/selinux/src/tools/xattr.rs Outdated Show resolved Hide resolved
experiment/selinux/src/selinux.rs Outdated Show resolved Hide resolved
Signed-off-by: Hiroyuki Moriya <[email protected]>
@Gekko0114
Copy link
Contributor Author

Thanks @YJDoc2,

Please run clippy on local and check that all lints are passing, currently that is not getting verified in CI.

Sure, I ran clippy. BTW, why not introduing clippy in CI ?

I saw that for EINTR, we are returning it as-is to caller. If my understanding is correct EINTR indicates that syscall was interrupted and there might not be any actual error. In which case should we retry? in a loop? I think we had this discussion before somewhere, what was decided from that?

Though we didn't discuss it in great depth, we talked about this topic.
#2850 (comment)
In the xattr function, a loop is not used, but a loop is used in the label function that calls xattr.
https://github.com/Gekko0114/youki/blob/main/experiment/selinux/src/selinux_label.rs#L75
Does this answer your question?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 29, 2024

why not introduing clippy in CI ?

Clippy is in CI, but we have intentionally excluded experimental crates from cargo workspace because of which clippy does not run on those crates.

Does this answer your question?

Yep, that's exactly what I was looking for. In this case, ok we are handling it correctly.

Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

Thanks!

@YJDoc2 YJDoc2 added kind/experimental `/experimental` and removed kind/feature labels Sep 29, 2024
@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 29, 2024

I re-labeled this to experimental, as we have still kept the crate in experimental. Once we move this and start using it in youki, that PR should be marked as feature.

@YJDoc2 YJDoc2 merged commit ab6c074 into containers:main Sep 29, 2024
29 checks passed
@github-actions github-actions bot mentioned this pull request Sep 29, 2024
@Gekko0114 Gekko0114 deleted the fix/selinux branch September 29, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/experimental `/experimental`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants