Refactor some complex functions #1832

giuseppe · 2025-07-21T20:28:33Z

More details in each commit.

🤖 Assisted by: https://claude.ai/code

Summary by Sourcery

Refactor and modularize core container and mount logic by extracting large monolithic functions into smaller helper routines, improving readability and maintainability.

Enhancements:

Use xmalloc0 in get_yajl_result for simplified allocation and zero‐initialization
Split container_init_setup into focused helpers for rootfs resolution, environment setup, terminal configuration, executable lookup, and security application
Introduce setup_* helpers in libcrun_container_run_internal to handle hooks output, keyring, terminal sockets, seccomp setup, console socket, and cgroup manager selection
Refactor mount processing in linux.c by extracting process_single_mount and related routines to uniformly handle sysfs/proc/, bind mounts, tmpfs, cgroup mounts, symlink copying, and tmpcopyup operations

sourcery-ai · 2025-07-21T20:28:38Z

Reviewer's Guide

This PR refactors large and complex functions in container.c and linux.c by extracting logical steps into dedicated static helpers, replaces manual memory allocation and OOM handling with a unified xmalloc0 wrapper, and modularizes mount operations for easier comprehension and maintenance.

Class diagram for refactored container initialization helpers

classDiagram
    class container_entrypoint_s
    class libcrun_container_t
    class runtime_spec_schema_config_schema
    class libcrun_error_t

    class container_init_setup {
        +int container_init_setup(void *args, pid_t own_pid, char *notify_socket, int sync_socket, char **exec_path, libcrun_error_t *err)
    }
    class resolve_rootfs_path {
        +int resolve_rootfs_path(runtime_spec_schema_config_schema *def, char **rootfs, libcrun_error_t *err)
    }
    class setup_terminal_socketpair {
        +int setup_terminal_socketpair(struct container_entrypoint_s *entrypoint_args, int *console_socketpair)
    }
    class setup_environment {
        +int setup_environment(runtime_spec_schema_config_schema *def, uid_t container_uid, libcrun_error_t *err)
    }
    class setup_terminal {
        +int setup_terminal(struct container_entrypoint_s *entrypoint_args, libcrun_container_t *container, int console_socket, int console_socketpair, int has_terminal, libcrun_error_t *err)
    }
    class setup_executable_path {
        +int setup_executable_path(struct container_entrypoint_s *entrypoint_args, runtime_spec_schema_config_schema *def, char **exec_path, libcrun_error_t *err)
    }
    class apply_security_settings {
        +int apply_security_settings(struct container_entrypoint_s *entrypoint_args, runtime_spec_schema_config_schema *def, libcrun_container_t *container, pid_t own_pid, libcrun_error_t *err)
    }

    container_init_setup --> resolve_rootfs_path
    container_init_setup --> setup_terminal_socketpair
    container_init_setup --> setup_environment
    container_init_setup --> setup_terminal
    container_init_setup --> setup_executable_path
    container_init_setup --> apply_security_settings

Class diagram for modularized mount operations in linux.c

classDiagram
    class do_mounts {
        +int do_mounts(libcrun_container_t *container, const char *rootfs, libcrun_error_t *err)
    }
    class process_single_mount {
        +int process_single_mount(libcrun_container_t *container, const char *rootfs, runtime_spec_schema_defs_mount *mount, struct libcrun_fd_map *mount_fds, size_t mount_index, const char *systemd_cgroup_v1, libcrun_error_t *err)
    }
    class handle_copy_symlink {
        +int handle_copy_symlink(libcrun_container_t *container, const char *rootfs, runtime_spec_schema_defs_mount *mount, libcrun_error_t *err)
    }
    class prepare_sysfs_or_proc_mount {
        +int prepare_sysfs_or_proc_mount(libcrun_container_t *container, const char *target, int *targetfd, libcrun_error_t *err)
    }
    class handle_tmpcopyup {
        +int handle_tmpcopyup(libcrun_container_t *container, const char *rootfs, const char *target, int copy_from_fd, libcrun_error_t *err)
    }
    class get_mount_label_how {
        +int get_mount_label_how(const char *type, bool is_sysfs_or_proc)
    }

    do_mounts --> process_single_mount
    process_single_mount --> handle_copy_symlink
    process_single_mount --> prepare_sysfs_or_proc_mount
    process_single_mount --> handle_tmpcopyup
    process_single_mount --> get_mount_label_how

File-Level Changes

Change	Details	Files
Use xmalloc0 for buffer allocation in JSON result generation	Replace malloc+null-check and OOM call with xmalloc0 in get_yajl_result	`src/libcrun/container.c`
Extract container initialization logic into setup_* helper functions	Move rootfs path resolution into resolve_rootfs_path Isolate terminal socketpair setup Factor out environment setup into setup_environment Pull terminal configuration into setup_terminal Separate executable path resolution into setup_executable_path Encapsulate security and capability application in apply_security_settings Simplify container_init_setup to invoke these helpers	`src/libcrun/container.c`
Modularize mount operations in linux.c	Introduce process_single_mount to handle each mount via handle_copy_symlink, prepare_sysfs_or_proc_mount, handle_tmpcopyup Extract filesystem validation in check_valid_no_follow_file_system and annotation lookup in get_force_cgroup_v1_annotation Refactor do_mounts to drive the loop through process_single_mount	`src/libcrun/linux.c`

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

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @giuseppe - I've reviewed your changes - here's some feedback:

The new setup_* helper functions in container.c could be moved into a dedicated initialization module to keep container.c from growing too large and improve readability.
In resolve_rootfs_path, consider explicitly handling or documenting the case where def->root is NULL so that callers always know whether *rootfs is set or must be defaulted.
process_single_mount remains quite long and covers multiple distinct cases; splitting bind, tmpcopyup, and sysfs/proc logic into separate smaller functions would reduce complexity and improve maintainability.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The new setup_* helper functions in container.c could be moved into a dedicated initialization module to keep container.c from growing too large and improve readability.
- In resolve_rootfs_path, consider explicitly handling or documenting the case where def->root is NULL so that callers always know whether *rootfs is set or must be defaulted.
- process_single_mount remains quite long and covers multiple distinct cases; splitting bind, tmpcopyup, and sysfs/proc logic into separate smaller functions would reduce complexity and improve maintainability.

## Individual Comments

### Comment 1
<location> `src/libcrun/container.c:1372` </location>
<code_context>
-          close_and_reset (&console_socketpair);
-        }
-    }
+  ret = setup_terminal (entrypoint_args, container, console_socket, console_socketpair, has_terminal, err);
+  if (UNLIKELY (ret < 0))
+    return ret;
</code_context>

<issue_to_address>
setup_terminal does not close console_socket or console_socketpair on error.

Ensure that console_socket and console_socketpair are closed if setup_terminal fails to prevent resource leaks. Clarify ownership if necessary.
</issue_to_address>

### Comment 2
<location> `src/libcrun/linux.c:2277` </location>
<code_context>
         }
+    }

-      if (extra_flags & OPTION_TMPCOPYUP)
-        {
-          if (strcmp (type, "tmpfs") != 0)
</code_context>

<issue_to_address>
handle_tmpcopyup may not handle ENOTDIR for non-directories.

Please verify if ignoring ENOTDIR is appropriate for all mount types when OPTION_TMPCOPYUP is set, or if further validation should be added.
</issue_to_address>

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

sourcery-ai · 2025-07-21T20:30:20Z

src/libcrun/container.c

-          close_and_reset (&console_socketpair);
-        }
-    }
+  ret = setup_terminal (entrypoint_args, container, console_socket, console_socketpair, has_terminal, err);


issue (bug_risk): setup_terminal does not close console_socket or console_socketpair on error.

Ensure that console_socket and console_socketpair are closed if setup_terminal fails to prevent resource leaks. Clarify ownership if necessary.

sourcery-ai · 2025-07-21T20:30:20Z

src/libcrun/linux.c

-          ret = safe_create_symlink (get_private_data (container)->rootfsfd, rootfs, target, def->mounts[i]->destination, err);
+      if (S_ISLNK (src_mode) && (extra_flags & OPTION_DEST_NOFOLLOW) && source_mountfd < 0)
+        {
+          ret = get_bind_mount (AT_FDCWD, mount->source, true, true, extra_flags & OPTION_SRC_NOFOLLOW, err);
          if (UNLIKELY (ret < 0))
            return ret;



question: handle_tmpcopyup may not handle ENOTDIR for non-directories.

Please verify if ignoring ENOTDIR is appropriate for all mount types when OPTION_TMPCOPYUP is set, or if further validation should be added.

giuseppe · 2025-07-22T08:33:56Z

@kolyshkin @flouthoc PTAL

flouthoc

LGTM

flouthoc

Restarted failing test.

kolyshkin

Only left one nit for the first commit, need more time to review the rest as it's lots of changes without much description. Will take a closer look tomorrow.

Also, from the quick glance it seems that commit 4 ("container: do not call libcrun_set_io_priority twice") needs to be before commit 3 ("container: refactor libcrun_container_run_internal"), unless it fixes an issue in commit 3, in which case these two may better be squashed?

kolyshkin · 2025-07-23T05:12:33Z

src/libcrun/container.c

-  *out = malloc (buf_len + 1);
-  if (*out == NULL)
-    OOM ();
+  *out = xmalloc0 (buf_len + 1);


nit: shouldn't this be xmalloc not xmalloc0?

yes, that is better. Fixed now

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2025-07-23T08:38:55Z

Also, from the quick glance it seems that commit 4 ("container: do not call libcrun_set_io_priority twice") needs to be before commit 3 ("container: refactor libcrun_container_run_internal"), unless it fixes an issue in commit 3, in which case these two may better be squashed?

I don't think we even need it, it was a mistake and I've dropped the patch from the PR. Thanks for noticing it!

packit-as-a-service · 2025-07-23T09:03:04Z

Ephemeral COPR build failed. @containers/packit-build please check.

flouthoc

LGTM

sourcery-ai bot approved these changes Jul 21, 2025

View reviewed changes

flouthoc approved these changes Jul 23, 2025

View reviewed changes

flouthoc reviewed Jul 23, 2025

View reviewed changes

kolyshkin reviewed Jul 23, 2025

View reviewed changes

giuseppe added 4 commits July 23, 2025 10:38

container: use xmalloc instead of malloc and OOM

f9edd52

Signed-off-by: Giuseppe Scrivano <[email protected]>

linux: refactor do_mounts

e83abf3

Signed-off-by: Giuseppe Scrivano <[email protected]>

container: refactor libcrun_container_run_internal

7acad11

Signed-off-by: Giuseppe Scrivano <[email protected]>

container: refactor container_init_setup

e76c745

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the reduce-complexity branch from d1e91d1 to e76c745 Compare July 23, 2025 08:38

flouthoc approved these changes Jul 23, 2025

View reviewed changes

flouthoc merged commit 0cf6292 into containers:main Jul 23, 2025
30 of 42 checks passed

github-actions bot mentioned this pull request Jul 27, 2025

Bump crun from 1.14.4 to 1.23 kokyhm/kubespray#258

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor some complex functions #1832

Refactor some complex functions #1832

Uh oh!

giuseppe commented Jul 21, 2025 •

edited by sourcery-ai bot

Loading

Uh oh!

sourcery-ai bot commented Jul 21, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Uh oh!

sourcery-ai bot Jul 21, 2025

Uh oh!

sourcery-ai bot Jul 21, 2025

Uh oh!

giuseppe commented Jul 22, 2025

Uh oh!

flouthoc left a comment

Uh oh!

flouthoc left a comment

Uh oh!

kolyshkin left a comment

Uh oh!

kolyshkin Jul 23, 2025

Uh oh!

giuseppe Jul 23, 2025

Uh oh!

giuseppe commented Jul 23, 2025

Uh oh!

packit-as-a-service bot commented Jul 23, 2025

Uh oh!

flouthoc left a comment

Uh oh!

Uh oh!

Uh oh!

Refactor some complex functions #1832

Refactor some complex functions #1832

Uh oh!

Conversation

giuseppe commented Jul 21, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

Class diagram for refactored container initialization helpers

Class diagram for modularized mount operations in linux.c

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Jul 22, 2025

Uh oh!

flouthoc left a comment

Choose a reason for hiding this comment

Uh oh!

flouthoc left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

giuseppe Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Jul 23, 2025

Uh oh!

packit-as-a-service bot commented Jul 23, 2025

Uh oh!

flouthoc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

giuseppe commented Jul 21, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Jul 21, 2025 •

edited

Loading