Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

rootfs: consolidate mountpoint creation logic #4359

Merged

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Jul 25, 2024

The logic for how we create mountpoints is spread over each mountpoint preparation function, when in reality the behaviour is pretty uniform with only a handful of exceptions. So just move it all to one function that is easier to understand.

Signed-off-by: Aleksa Sarai cyphar@cyphar.com

The logic for how we create mountpoints is spread over each mountpoint
preparation function, when in reality the behaviour is pretty uniform
with only a handful of exceptions. So just move it all to one function
that is easier to understand.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
@cyphar cyphar requested review from kolyshkin, AkihiroSuda and rata July 25, 2024 04:16
@cyphar cyphar added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label Jul 25, 2024
@cyphar cyphar added this to the 1.1.14 milestone Jul 25, 2024
// but properly handling the case where path or root are "/".
//
// NOTE: The return value only make sense if the path doesn't contain "..".
func IsLexicallyInRoot(root, path string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have unit tests?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with adding them later, if that helps.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

me := mountEntry{Mount: m}
// For all other filesystems, just make the target.
if _, err := createMountpoint(c.config.Rootfs, me); err != nil {
return fmt.Errorf("create criu restore mountpoint for %s mount: %w", me.Destination, err)
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we want to use %q for the destination here? It quotes the string, in case it has spaces and so

@AkihiroSuda AkihiroSuda merged commit 3d7bc3b into opencontainers:main Jul 29, 2024
40 checks passed
@cyphar cyphar deleted the rootfs-create-mountpoint-refactor branch July 30, 2024 06:28
@lifubang lifubang mentioned this pull request Aug 31, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
backport/1.1-todo A PR in main branch which needs to be backported to release-1.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants