Skip to content

oci: appendOSMounts(): remove unused error, and move#7874

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
thaJeztah:appendOSMounts_error
Dec 29, 2022
Merged

oci: appendOSMounts(): remove unused error, and move#7874
dmcgowan merged 1 commit intocontainerd:mainfrom
thaJeztah:appendOSMounts_error

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Dec 27, 2022

This function was added in ae22854 (#7000), but never returned an error, and the error-return was not handled on the callsite. This patch removes the unused error return, and moves it to a file related to mounts, which allowed for some of the stubs to be removed and shared between non-FreeBSD platforms.

@thaJeztah
oci: appendOSMounts(): remove unused error, and move
94c68aa 
This function was added in ae22854, but never
returned an error, and the error-return was not handled on the callsite. This
patch removes the unused error return, and moves it to a file related to mounts,
which allowed for some of the stubs to be removed and shared between non-FreeBSD
platforms.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah requested a review from samuelkarp December 27, 2022 09:26
dmcgowan
dmcgowan approved these changes Dec 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

The //go:build !freebsd in mounts.go is still a bit confusing and makes more work for reviewing changes in these files.

@fuweid fuweid mentioned this pull request Dec 28, 2022
Closed
@fuweid fuweid added the platform/freebsd FreeBSD label Dec 28, 2022
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Dec 28, 2022

The //go:build !freebsd in mounts.go is still a bit confusing and makes more work for reviewing changes in these files.

Yeah, it's a bit confusing indeed; perhaps the file should be renamed to mounts_nofreebsd.go to make it more apparent 🤔

@fangn2
Copy link
Copy Markdown
Contributor

fangn2 commented Dec 28, 2022

Yeah, it's a bit confusing indeed; perhaps the file should be renamed to mounts_nofreebsd.go to make it more apparent 🤔

How about mounts_other.go?
A search of this pattern in the containerd repo gives a lot of similar use cases.

$ find . -name *_other.go
./vendor/golang.org/x/sys/unix/sysvshm_unix_other.go
./vendor/golang.org/x/sys/unix/sockcmsg_unix_other.go
./vendor/golang.org/x/term/term_unix_other.go
./vendor/github.com/klauspost/compress/internal/snapref/decode_other.go
./vendor/github.com/klauspost/compress/internal/snapref/encode_other.go
./vendor/github.com/klauspost/compress/zstd/internal/xxhash/xxhash_other.go
./vendor/github.com/cilium/ebpf/internal/unix/types_other.go
./vendor/github.com/godbus/dbus/v5/conn_other.go
./vendor/github.com/minio/sha256-simd/cpuid_other.go
./vendor/github.com/minio/sha256-simd/sha256block_other.go
./vendor/github.com/containerd/nri/pkg/runtime-tools/generate/helpers_other.go
./vendor/github.com/containerd/nri/pkg/adaptation/plugin_other.go
./vendor/github.com/containerd/go-runc/command_other.go
./vendor/github.com/containerd/continuity/testutil/mount_other.go
./vendor/github.com/intel/goresctrl/pkg/cgroups/cgroupid_other.go
./vendor/github.com/fsnotify/fsnotify/backend_other.go
./vendor/github.com/container-orchestrated-devices/container-device-interface/pkg/cdi/spec_other.go
./vendor/github.com/cespare/xxhash/v2/xxhash_other.go
./vendor/github.com/prometheus/client_golang/prometheus/process_collector_other.go
./plugin/plugin_other.go
./pkg/nri/container_other.go
./pkg/nri/sandbox_other.go
./pkg/cri/server/container_create_other.go
./pkg/cri/server/sandbox_stats_other.go
./pkg/cri/server/sandbox_run_other.go
./pkg/cri/server/sandbox_portforward_other.go
./pkg/cri/server/helpers_other.go
./pkg/cri/server/container_update_resources_other.go
./pkg/cri/server/nri-api_other.go
./pkg/cri/server/container_stats_list_other.go
./pkg/cri/server/service_other.go
./pkg/cri/sbserver/container_create_other.go
./pkg/cri/sbserver/sandbox_stats_other.go
./pkg/cri/sbserver/sandbox_run_other.go
./pkg/cri/sbserver/sandbox_portforward_other.go
./pkg/cri/sbserver/helpers_other.go
./pkg/cri/sbserver/podsandbox/sandbox_run_other.go
./pkg/cri/sbserver/podsandbox/helpers_other.go
./pkg/cri/sbserver/container_update_resources_other.go
./pkg/cri/sbserver/container_stats_list_other.go
./pkg/cri/sbserver/service_other.go
./pkg/seed/seed_other.go
./pkg/os/mount_other.go
./pkg/testutil/mount_other.go
./pkg/netns/netns_other.go
./platforms/platforms_other.go
./rootfs/init_other.go
./snapshots/testsuite/helpers_other.go
./diff/apply/apply_other.go
./cmd/ctr/commands/namespaces/namespaces_other.go

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Dec 29, 2022

A file rename can be done in another change if it makes sense. mount_other.go might make more sense, but also a bit confusing since most people don't look in there unless they are expecting to make changes for a less common "other" platform.

@dmcgowan dmcgowan merged commit 729206f into containerd:main Dec 29, 2022
@thaJeztah thaJeztah deleted the appendOSMounts_error branch December 29, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@fuweid fuweid fuweid approved these changes

@samuelkarp samuelkarp Awaiting requested review from samuelkarp

Assignees

No one assigned

Labels

platform/freebsd FreeBSD

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @fangn2 @dmcgowan @fuweid