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

files: track relabeling using set instead of slice #1324

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

Nemric
Copy link
Contributor

@Nemric Nemric commented Mar 1, 2022

This PR handle relabelling by preventing multiple relabels ...
The goal is to avoid managing it in the code allowing contributors to call relabelling func without having to check if the path is already included in the "torelabel" slice

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I'm not sure if this is worth it. setfiles will handle duplicate entries just fine and in general we shouldn't be generating that many duplicates anyway.

If we really want to make sure that all paths are unique before passing to setfiles, maybe a cleaner approach would be to switch from a slice to a set (really, a map[string]struct{}).

@Nemric
Copy link
Contributor Author

Nemric commented Mar 1, 2022

Yes I understand, so in #1303 I used to code differents things to prevent sending to relabel too many file/path
Bgilbert ask me for a better way to do that by finding the "root" of setting file/path to relabel, that's what this PR is intended to do.
if I well understand, the "action" for relabelling (setfiles) already handle this case ?

@jlebon
Copy link
Member

jlebon commented Mar 2, 2022

Yes, setfiles itself will work just fine as is.

OK, read some of the discussions in #1303 now. That PR is introducing code which will make it easier to have multiple requests for relabeling the same dirs unless the code is made more complex to handle that.

So I'm cool with embedding handling for this into the relabeling bits directly. For that, let's do

maybe a cleaner approach would be to switch from a slice to a set (really, a map[string]struct{}).

instead?

@jlebon
Copy link
Member

jlebon commented Mar 2, 2022

That said, I'd consider this orthogonal to #1303. You can go ahead and simplify the code there to just call s.relabel() "inefficiently" for now and we don't have to block it on this necessarily. (Big picture, we're not talking about that many duplicates here, right? Something like user unit * (users for unit - 1) summed across all user units?)

@Nemric
Copy link
Contributor Author

Nemric commented Mar 2, 2022

Yes, we don't speak about a ton of files, you're right :) I just dive in the systemd.unit code and I don't know the amount of files/paths that have to be relabelled when setfiles is called
When I went into debug mode, I saw some duplicates, and we (@bgilbert and I) started to avoid that. (e.g. : #1318 )

So I'm cool with embedding handling for this into the relabeling bits directly. For that, let's do

maybe a cleaner approach would be to switch from a slice to a set (really, a map[string]struct{}).

instead?

I agree for using map against slice but to be honest, I felt lazy handling that 😄 will give it a try ;)

@Nemric Nemric mentioned this pull request Mar 3, 2022
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Looks close! Can you also squash all your commits to a single one?

@@ -132,7 +132,7 @@ func (s *stage) checkRelabeling() error {

// initialize to non-nil (whereas a nil slice means not to append, even
Copy link
Member

Choose a reason for hiding this comment

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

Outdated comment. I think the point it makes is still valid for maps, so you can just s/slice/map/.

@@ -163,5 +163,9 @@ func (s *stage) relabelFiles() error {
// loaded and hence no MAC enforced, and (2) we'd still need after-the-fact
// labeling for files created by processes we call out to, like `useradd`.

return s.RelabelFiles(s.toRelabel)
keys := []string{}
Copy link
Member

Choose a reason for hiding this comment

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

Minor: we can use make here instead to preset the capacity since we know how many things we'll add.

@Nemric Nemric force-pushed the Relabeling branch 2 times, most recently from dd1bc49 to ebaa437 Compare March 4, 2022 06:57
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Thanks!

@jlebon
Copy link
Member

jlebon commented Mar 4, 2022

Sorry, one final thing. Let's tweak the commit message to something like:

files: track relabeling using set instead of slice

That way we avoid passing the same path to `setfiles` multiple times.

?

That way we avoid passing the same path to `setfiles` multiple times.
@jlebon jlebon changed the title Handling multiple relabelling files: track relabeling using set instead of slice Mar 4, 2022
@jlebon jlebon enabled auto-merge March 4, 2022 16:38
@jlebon jlebon merged commit 8cb3a69 into coreos:main Mar 4, 2022
@Nemric Nemric deleted the Relabeling branch March 4, 2022 20:06
@dustymabe
Copy link
Member

Nice work @Nemric!

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.

None yet

3 participants