Renaming Mappings fields and use int for mountEntry.fd #3939

eiffel-fl · 2023-07-18T09:06:05Z

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.

rata

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.

libcontainer/rootfs_linux.go

libcontainer/container_linux.go

fuweid · 2023-07-20T07:07:12Z

libcontainer/configs/config_linux.go

@@ -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 {


maybe we should use len(c.UIDMappings) == 0 here?

@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.

It seems like we didn't solve it but marked as resolved. Did I miss something here?

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.

#3939 (review)

The new PR is here: #3947

libcontainer/container_linux.go

thaJeztah

overall, I like this!

I left some comments, but nothing critical afaics

thaJeztah · 2023-07-20T10:35:33Z

libcontainer/configs/config_linux.go

@@ -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 {


Very tiny nit: this could be simplified to just;

Suggested change

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!") } }

Thank you for the clarification! I always forgot about this!

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/configs/config_linux.go

thaJeztah · 2023-07-20T10:45:58Z

libcontainer/rootfs_linux.go

@@ -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


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 ?

@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* :)

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 😂

/proc/self/fd/-123 would be invalid

Yeah. But it might be easy to debug I think 🤔

thaJeztah

LGTM

@kolyshkin PTAL

libcontainer/rootfs_linux.go

libcontainer/configs/validate/rootless_test.go

libcontainer/rootfs_linux.go

libcontainer/configs/validate/rootless_test.go

Signed-off-by: Francis Laniel <[email protected]>

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 <[email protected]>

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 <[email protected]>

lifubang

LGTM
Thanks!

eiffel-fl · 2023-07-21T15:33:33Z

LGTM Thanks!

Thank you and sorry for the misunderstanding 😅!

kolyshkin

LGTM, thanks!

eiffel-fl · 2023-07-21T16:19:56Z

Awesome! Thank you for the merge!

eiffel-fl force-pushed the francis/caps-naming branch 7 times, most recently from 9a7cce4 to 2e9093f Compare July 18, 2023 12:42

eiffel-fl changed the title ~~Renaming Mappings field and use int for srcFD~~ Renaming *Mappings fields and use int for srcFD Jul 18, 2023

rata approved these changes Jul 19, 2023

View reviewed changes

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

kolyshkin reviewed Jul 19, 2023

View reviewed changes

libcontainer/container_linux.go Outdated Show resolved Hide resolved

fuweid reviewed Jul 20, 2023

View reviewed changes

eiffel-fl force-pushed the francis/caps-naming branch 2 times, most recently from ade2907 to 5ef5f41 Compare July 20, 2023 10:07

eiffel-fl changed the title ~~Renaming *Mappings fields and use int for srcFD~~ Renaming *Mappings fields and use int* for mountEntry.fd Jul 20, 2023

thaJeztah reviewed Jul 20, 2023

View reviewed changes

eiffel-fl force-pushed the francis/caps-naming branch from 5ef5f41 to 2393b1c Compare July 20, 2023 11:19

thaJeztah approved these changes Jul 20, 2023

View reviewed changes

lifubang requested changes Jul 20, 2023

View reviewed changes

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

lifubang reviewed Jul 20, 2023

View reviewed changes

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/caps-naming branch from 2393b1c to 50e528f Compare July 20, 2023 15:13

lifubang requested changes Jul 21, 2023

View reviewed changes

libcontainer/configs/validate/rootless_test.go Outdated Show resolved Hide resolved

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/caps-naming branch from 50e528f to 3f02311 Compare July 21, 2023 07:46

lifubang reviewed Jul 21, 2023

View reviewed changes

libcontainer/configs/validate/rootless_test.go Outdated Show resolved Hide resolved

lifubang reviewed Jul 21, 2023

View reviewed changes

libcontainer/configs/validate/rootless_test.go Outdated Show resolved Hide resolved

lifubang reviewed Jul 21, 2023

View reviewed changes

libcontainer/configs/validate/rootless_test.go Outdated Show resolved Hide resolved

eiffel-fl force-pushed the francis/caps-naming branch 4 times, most recently from 2de0421 to c09eabb Compare July 21, 2023 11:38

lifubang requested changes Jul 21, 2023

View reviewed changes

eiffel-fl added 3 commits July 21, 2023 13:55

Capitalize [UG]idMappings as [UG]IDMappings

c47f58c

Signed-off-by: Francis Laniel <[email protected]>

Remove idmapFD field for mountEntry

a3785c8

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 <[email protected]>

eiffel-fl force-pushed the francis/caps-naming branch from c09eabb to a3785c8 Compare July 21, 2023 11:55

lifubang approved these changes Jul 21, 2023

View reviewed changes

kolyshkin approved these changes Jul 21, 2023

View reviewed changes

kolyshkin merged commit 74895d4 into opencontainers:main Jul 21, 2023
36 checks passed

eiffel-fl deleted the francis/caps-naming branch July 21, 2023 16:19

This was referenced Jul 21, 2023

consider unifying mountEntry.srcFD and mountEntry.idmapFD approach #3935

Open

Define documented const for magic -1 file-descriptor #3934

Closed

lifubang added this to the 1.2.0 milestone Jul 22, 2023

lifubang mentioned this pull request Jul 23, 2023

use the length of UIDMappings/GIDMappings to check whether empty or not #3947

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming Mappings fields and use int for mountEntry.fd #3939

Renaming Mappings fields and use int for mountEntry.fd #3939

eiffel-fl commented Jul 18, 2023

rata left a comment

fuweid Jul 20, 2023

rata Jul 20, 2023

fuweid Jul 23, 2023

lifubang Jul 23, 2023

lifubang Jul 23, 2023

lifubang Jul 23, 2023

thaJeztah left a comment

thaJeztah Jul 20, 2023

eiffel-fl Jul 20, 2023

thaJeztah Jul 20, 2023

thaJeztah Jul 20, 2023

rata Jul 20, 2023

thaJeztah Jul 20, 2023

fuweid Jul 21, 2023

thaJeztah left a comment

lifubang left a comment

eiffel-fl commented Jul 21, 2023

kolyshkin left a comment

eiffel-fl commented Jul 21, 2023

	if c.UIDMappings == nil \|\| len(c.UIDMappings) == 0 {
	if len(c.UIDMappings) == 0 {

Renaming *Mappings fields and use int* for mountEntry.fd #3939

Renaming *Mappings fields and use int* for mountEntry.fd #3939

Conversation

eiffel-fl commented Jul 18, 2023

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

lifubang left a comment

Choose a reason for hiding this comment

eiffel-fl commented Jul 21, 2023

kolyshkin left a comment

Choose a reason for hiding this comment

eiffel-fl commented Jul 21, 2023

Renaming Mappings fields and use int for mountEntry.fd #3939

Renaming Mappings fields and use int for mountEntry.fd #3939