-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Renaming *Mappings fields and use int* for mountEntry.fd #3939
Conversation
9a7cce4
to
2e9093f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM.
I added one comment of another simplification that can be done on top of this (don't amend this commit).
Curious what @thaJeztah thinks about this.
@@ -13,10 +13,10 @@ var ( | |||
// different when user namespaces are enabled. | |||
func (c Config) HostUID(containerId int) (int, error) { | |||
if c.Namespaces.Contains(NEWUSER) { | |||
if c.UidMappings == nil { | |||
if c.UIDMappings == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should use len(c.UIDMappings) == 0
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eiffel-fl the ==nil should be removed. If it is nil the len is always 0. If it is not nil, the length can be 0 too.
So no need to keep both checks, we can just check for the len and that is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we didn't solve it but marked as resolved. Did I miss something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe forgot at that time, thank you for your remind. This PR addressed too much problems in ONE PR(combine FD fields and rename *idMapping). Feel free to open an new PR to resolve this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new PR is here: #3947
ade2907
to
5ef5f41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, I like this!
I left some comments, but nothing critical afaics
libcontainer/configs/config_linux.go
Outdated
@@ -13,10 +13,10 @@ var ( | |||
// different when user namespaces are enabled. | |||
func (c Config) HostUID(containerId int) (int, error) { | |||
if c.Namespaces.Contains(NEWUSER) { | |||
if c.UidMappings == nil { | |||
if c.UIDMappings == nil || len(c.UIDMappings) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very tiny nit: this could be simplified to just;
if c.UIDMappings == nil || len(c.UIDMappings) == 0 { | |
if len(c.UIDMappings) == 0 { |
a nil
slice has length 0
; https://go.dev/play/p/tSRvH2e-yH0
package main
import "fmt"
func main() {
var foo []int = nil
if len(foo) == 0 {
fmt.Println("YUP!")
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification! I always forgot about this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries! There was nothing really wrong with the old code, just not really needed, so where possible, I try to keep things simple 🤗
libcontainer/rootfs_linux.go
Outdated
@@ -40,13 +40,12 @@ type mountConfig struct { | |||
// mountEntry contains mount data specific to a mount point. | |||
type mountEntry struct { | |||
*configs.Mount | |||
srcFD string | |||
idmapFD int | |||
fd *int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious if (now that we no longer use magic -1
values) we needed to use an unsigned-integer, but it looks like unix.MoveMount()
expects an int
; https://github.com/golang/sys/blob/c406141231ada89c399c39dd52dd1e279c509f5c/unix/zsyscall_linux.go#L1272-L1288
func MoveMount(fromDirfd int, fromPathName string, toDirfd int, toPathName string, flags int) (err error) {
That said, mountViaFDs
does construct the /proc/self/fd/
path, so slightly wondering if we should have something unsigned.
Any thoughts @kolyshkin @fuweid @rata ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah I'd use *int
, fds are usually ints. I see we won't use the negative values now that we have a pointer to represent "don't use this", but if the functions we want to use take ints in any case, why would we want unsigned?
We might have bigger values stored in an unsiged int that we can represent with int (that is what several syscalls take). So, it seems like it can even create issues.
Am I missing something? I like int*
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not missing something directly, just thought that /proc/self/fd/-123
would be invalid, and having something unsigned could save us having to deal with that 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/proc/self/fd/-123 would be invalid
Yeah. But it might be easy to debug I think 🤔
5ef5f41
to
2393b1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@kolyshkin PTAL
2393b1c
to
50e528f
Compare
50e528f
to
3f02311
Compare
2de0421
to
c09eabb
Compare
Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
Previously to this commit, we used a string for srcFD as /proc/self/fd/NN. This commit modified to this behavior, so srcFD is only an *int and the full path is constructed in mountViaFDs() if srcFD is different than nil. Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
We cannot have both srcFD and idMapFD set at the same time. So, we can simplify this struct to only have one field which is used a srcFD most of the time and as idMapFD when we do an id map mount. Signed-off-by: Francis Laniel <flaniel@linux.microsoft.com>
c09eabb
to
a3785c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks!
Thank you and sorry for the misunderstanding 😅! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Awesome! Thank you for the merge! |
Hi.
This PR is a follow-up to #3717.
It fixes #3936 and #3935.
If you see any way to improve it, feel free to share.
Best regards.