Skip to content

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Sep 9, 2025

@sohankunkerkar PTAL

Summary by Sourcery

Implement unified handling of masked directory mounts for CRIU via a shared empty directory mechanism, introduce a utility to manage this directory, improve safety around state_root access, and fix the netlink buffer sizing in network setup

Enhancements:

  • Consolidate masked_paths handling in CRIU checkpoint and restore to support both file and directory mounts using a shared empty directory
  • Extract and reuse get_shared_empty_directory_path for creating and caching the shared empty directory
  • Add null checks around container context when accessing state_root in state directory, socket, device, and seccomp functions
  • Adjust netlink message buffer size in network configuration to use nlmsgerr structure

Copy link

sourcery-ai bot commented Sep 9, 2025

Reviewer's Guide

This PR tags the 1.24 release in NEWS and implements a major refactor of CRIU masked-path handling by introducing a shared empty-directory mount, centralizes the empty-directory path logic into a new helper, adds defensive null-checks for container context, corrects a network buffer size, and updates the public API accordingly.

File-Level Changes

Change Details Files
Enhanced CRIU masked-path checkpoint/restore logic with shared empty-directory support
  • Replaced single loop over masked_paths with branching for directories vs files
  • Mount one shared empty directory and bind-mask all dirs against it
  • Retained per-file mounts for regular masked files
src/libcrun/criu.c
Centralized empty-directory path creation into helper function
  • Extracted directory path creation and ensure-directory calls into get_shared_empty_directory_path
  • Removed redundant logic in get_shared_empty_dir_cached
src/libcrun/linux.c
src/libcrun/status.c
src/libcrun/status.h
Added defensive null-context checks when opening state and rundir
  • Guard calls to libcrun_get_state_directory with context existence
  • Guard calls to open_rundir_dirfd with context existence
src/libcrun/linux.c
src/libcrun/seccomp.c
Corrected network configuration buffer sizing
  • Changed local buffer from size of ifinfomsg to size of nlmsgerr
src/libcrun/linux.c
Updated NEWS for 1.24 release
  • Added NEWS entry for version 1.24
NEWS

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link

TMT tests failed. @containers/packit-build please check.

@giuseppe giuseppe marked this pull request as draft September 9, 2025 13:16
@giuseppe
Copy link
Member Author

giuseppe commented Sep 9, 2025

looks like there is a regression with CRIU, need to investigate it first

@giuseppe
Copy link
Member Author

giuseppe commented Sep 9, 2025

@sohankunkerkar it seems like the bind mount optimization is causing this issue

@giuseppe giuseppe force-pushed the tag-1.24 branch 2 times, most recently from b1c6dd5 to 0020f32 Compare September 9, 2025 17:03
}
}
{
cleanup_free char *empty_dir_path = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

We should create a helper function to dedup this logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

could you check the new version?

Copy link
Member

@sohankunkerkar sohankunkerkar left a comment

Choose a reason for hiding this comment

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

CI is green now! one nit but overall LGTM

@giuseppe giuseppe marked this pull request as ready for review September 9, 2025 18:25
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

commit 4004e5b introduced the
regression.  It is not part of any release.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@sohankunkerkar
Copy link
Member

/lgtm

@giuseppe giuseppe merged commit 6f0a3c5 into containers:main Sep 9, 2025
63 of 64 checks passed
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.

2 participants