src: various refactorings - #2186
Merged
Merged
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
giuseppe
force-pushed
the
refactor-reduce-complexity
branch
from
August 13, 2026 10:53
3bee13c to
9b62c32
Compare
Member
Author
|
@eriksjolund FYI: these were mostly found and fixed by Claude |
eriksjolund
reviewed
Aug 15, 2026
giuseppe
force-pushed
the
refactor-reduce-complexity
branch
from
August 15, 2026 08:43
9b62c32 to
0792db0
Compare
kolyshkin
reviewed
Aug 17, 2026
| if (UNLIKELY (r != json_gen_status_ok)) | ||
| goto gen_error; | ||
| GEN_KEY (gen, "ociVersion"); | ||
| GEN_KEY (gen, "1.0"); |
Collaborator
There was a problem hiding this comment.
nit: this should be GEN_STR.
kolyshkin
reviewed
Aug 17, 2026
| int (*idmap_helper) (pid_t, const char *, libcrun_error_t *), const char *helper_name, | ||
| bool warn_on_failure, const char *map, size_t map_len, | ||
| runtime_spec_schema_defs_id_mapping **mappings, size_t mappings_len, | ||
| uint32_t host_id, uint32_t container_id, bool honor_deny_setgroups_flag, |
Collaborator
There was a problem hiding this comment.
I'd rather s/honor_deny_setgroups_flag/is_uid/
Collaborator
|
Just a couple of nits, otherwise LGTM. @eriksjolund PTAL (and if you're fine with my proposed changes I'll force push and merge this). |
Member
Author
Your suggestions LGTM |
Introduce GEN_OR_FAIL/GEN_KEY/GEN_STR macros to replace the repeated 'r = json_gen_*(...); if (UNLIKELY (r != json_gen_status_ok)) goto ...;' pattern that appeared dozens of times in do_hooks and get_seccomp_receiver_fd_payload. Also apply them in libcrun_container_state, which previously ignored all json_gen_* return values, so JSON generation errors are now reported consistently. No functional change other than libcrun_container_state now checking for generation errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The annotations map serialization loop was copy-pasted in do_hooks, get_seccomp_receiver_fd_payload and libcrun_container_state. Extract it into a single gen_annotations helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
exec_process_entrypoint applied the seccomp profile with two nearly identical blocks, one before and one after setting the capabilities (depending on process->no_new_privileges). Extract the common logic into apply_seccomp_for_exec. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
wait_for_process and libcrun_container_restore contained identical code to format a pid into a fixed buffer and write it to the pid file. Extract it into write_pid_file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
libcrun_container_pause, libcrun_container_unpause and libcrun_container_checkpoint shared the same 'read status, check it is running, otherwise error out' preamble. Extract it into a helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
libcrun_container_state, libcrun_container_exec_with_options and libcrun_container_update_intel_rdt each reimplemented the get_state_directory + append_paths(config.json) + load_from_file sequence inline. Route them through the existing read_container_config_from_state helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The forked children in libcrun_container_run and libcrun_container_exec_with_options reported failures to the parent using the same wire format (int errno followed by a NUL-terminated message). Extract write_error_to_pipe to centralize the format. The two readers are left as-is since they have different semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
In do_hooks the json_gen_ctx is always non-NULL after json_gen_init succeeds, so the 'if (gen)' guards on the two exit paths were dead code. Manage the context with a cleanup attribute (cleanup_json_gen), which removes the duplicated json_gen_free calls and simplifies the error path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The uid and gid halves of libcrun_set_usernamespace were near-identical: try newXidmap, on failure write the map file, and on write failure fall back to a single mapping guarded by deny_setgroups. Extract a single write_id_map helper parameterized on the map name, the id-map helper, the mappings array, and the host/container ids. The one real difference between the two paths is preserved via the honor_deny_setgroups_flag argument: the uid path only calls deny_setgroups when it has not already been done (the gid path, which runs first, always calls it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The block that pre-opens the needed device fds and the notify socket via open_tree (before the host file system becomes unreachable) was duplicated verbatim in the tree_fd >= 0 and tree_fd < 0 paths of the mount-namespace setup. Extract it into a single helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Move the mount-propagation handling (mount_setattr with a mount(2) fallback) out of the already very large do_mount into a small helper. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Move the rootfs-replacement branch (re-enter the mount namespace and reopen the rootfs after a mount on "/") out of do_mount into a helper. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Move the best-effort security.selinux xattr application (LABEL_XATTR) out of do_mount into a helper, keeping it under HAVE_FGETXATTR as before. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Move the remount handling (immediate remount vs. deferred remount queued onto the remounts list) out of do_mount into a helper. No behavior change: the deferred path still returns success and do_mount returns the same value as before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
When the modify_oci_configuration handler failed, the error path did `return ret` directly instead of going through the cleanup label, leaking doc, parser_err, and resources. Route it through `goto cleanup` like every other error exit in the function. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The GEN_OR_FAIL/GEN_KEY/GEN_STR macros and the cleanup_json_gen attribute were local to container.c, but the same json_gen_* boilerplate appears in other files. Move them to a new json_gen_utils.h so they can be reused. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Apply the shared GEN_OR_FAIL/GEN_KEY/GEN_STR macros and the cleanup_json_gen attribute in the status-file writer, replacing the repeated `r = json_gen_*(...); if (...) goto gen_error;` boilerplate and dropping the manual json_gen_free in both exit paths. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
get_state_directory_status_file open-coded validate_id + get_run_directory + append_paths, which is exactly what libcrun_get_state_directory already does. Call it and append the "status" component. Same resulting path, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The three exec.fifo functions (create/write/has-read) shared the same
get_state_directory + append_paths("exec.fifo") preamble. Extract it into
a single helper. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
`resources` is unconditionally dereferenced earlier in the function (has_allow_all (resources->devices, ...)), and the only caller, append_resources, already returns early when resources is NULL. The `if (resources == NULL)` guard is therefore unreachable. Remove it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
store_seccomp_cache and libcrun_open_seccomp_bpf shared the same block: open the run-directory dirfd and build the "<id>/seccomp.bpf" path relative to it. Extract it into one helper. This also removes the dead `container->context ? ... : NULL` ternary from libcrun_open_seccomp_bpf (it early-returns on a NULL context) while keeping the guard where store_seccomp_cache needs it. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
append_env and append_cap were identical except for the target field. Collapse them onto a shared append_to_string_array helper. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The "append suffix to slice, or dup it" block in systemd_finalize was duplicated for the v1 and v2 branches. Factor it into a small helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The three D-Bus call paths (enter_systemd_cgroup_scope and the two systemd property setters) ended with an identical exit: teardown that unref'd bus/message/reply and freed the sd_bus_error. Factor it into a helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The three "is this device already in the spec?" scan loops (/dev/kvm, /dev/sev, /dev/nitro_enclaves) were identical apart from the path. Factor them into a small predicate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The three stat() blocks in libkrun_modify_oci_configuration handled the "missing device is fine, other errors fail" case identically. Factor them into a helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The work-directory setup (mkdir + open + criu_set_work_dir_fd, or fall back to the images dir) was duplicated between checkpoint and restore. Factor it into a helper; the opened fd is returned to the caller so its cleanup_close lifetime is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
restore_cgroup_v1_mount and checkpoint_cgroup_v1_mount both parsed a
/proc/self/cgroup line into subsystem/subpath and applied the same name
normalization ("name=" stripping and the net_cls/cpuacct remaps).
Factor it into a helper; the subpath output is optional since the
checkpoint path does not need it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
make_oci_process_user duplicated the strtoll + range/format validation
for the UID and GID, which was also nearly identical to the existing
parse_int_or_fail helper.
Merge them into a single parse_id_or_fail (in crun.c) with an optional
endptr argument: when NULL the whole string must be a valid number (the
preserve-fds and LISTEN_FDS callers), otherwise *endptr is advanced past
the parsed number so the caller can continue parsing a "UID:GID" pair.
Route make_oci_process_user and all former parse_int_or_fail callers
through it.
The preserve-fds/LISTEN_FDS errors are now worded like the rest
("invalid <field> specified") and an empty string is rejected instead
of being parsed as 0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
kolyshkin
force-pushed
the
refactor-reduce-complexity
branch
from
August 17, 2026 23:29
0792db0 to
88e554f
Compare
Collaborator
Implemented and force-pushed here. Still LGTM. |
Contributor
|
lgtm (my review just concerns 88e554f "exec: unify numeric parsing into parse_id_or_fail") |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
a series of cleanups, more comments in each patch