Various small fixes and reverse dependency tests using packit#166
Conversation
📝 WalkthroughWalkthroughAdds a new CodeRabbit config and Packit CI jobs/metadata, updates C size-calculation code and header (allocation assertion, error handling, new error code), and applies non-functional comment/whitespace edits to tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.packit.yaml (1)
114-115: Remove mpfr-devel from srpm_build_deps in .packit.yaml.PR
#163switched to rational arithmetic and eliminated the mpfr dependency. The spec file and configure.ac no longer require mpfr-devel, but it remains listed in.packit.yamlat line 115. Remove it to keep the build configuration in sync with actual requirements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.fmf/versionis excluded by!.fmf/**dist/libbytesize.spec.inis excluded by!**/dist/**plans/blivet.fmfis excluded by!plans/**plans/tests.fmfis excluded by!plans/**
📒 Files selected for processing (3)
.coderabbit.yaml.packit.yamlsrc/bs_size.c
🔇 Additional comments (5)
src/bs_size.c (2)
703-707: LGTM! Memory leak fix on error path.The
mpz_clear(max)call properly releases the GMP integer allocated at line 693 before returning. This prevents a memory leak when the size exceedsUINT64_MAX.
1214-1217: LGTM! Correct return value for pointer type.Returning
NULLinstead of0is semantically correct for a function returningchar*. This also aligns with the error handling pattern at line 1255 and the function documentation indicating it returns a string on success..coderabbit.yaml (1)
1-14: LGTM! Valid CodeRabbit configuration.The configuration file conforms to the schema with sensible defaults for a "chill" review profile. The
path_filterscorrectly exclude translation files (po/), plans, GitHub workflows, and FMF metadata from reviews, which is appropriate for reducing noise on non-functional files..packit.yaml (2)
80-87: LGTM! Standard pull request tests configuration.The test job correctly targets both latest and latest-stable Fedora releases on x86_64 and aarch64 architectures, providing good coverage for PR validation.
89-106: Good reverse dependency testing setup for Blivet.The
revdeps_blivetjob configuration is well-structured:
- Uses an identifier to distinguish from the basic tests job
- Includes failure notifications to alert the maintainer
- Properly configures the Blivet daily COPR repository for testing against latest Blivet
The TMT context variable
revdeps_blivet: "yes"will enable the corresponding test plans.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/bs_size.c`:
- Around line 1216-1219: The division-by-zero handling is inconsistent:
bs_size_div (around the mpz_cmp_ui check) returns NULL on error but
bs_size_true_div_int still returns 0; change bs_size_true_div_int so that when
divisor == 0 it calls set_error (as it already does) and returns NULL (not 0) to
match the char* error-return convention used by bs_size_div and the function's
return type; update only the return value in bs_size_true_div_int to NULL,
keeping the existing set_error call and message.
- Line 10: In bs_size_true_div_int(), the division-by-zero error path currently
returns the integer literal 0 even though the function's return type is char*;
change the return value to NULL to match the function signature and the existing
pattern used in bs_size_true_div() (keep the set_error call and strdup_printf
message intact).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/bs_size.csrc/bs_size.htests/libbytesize_unittest.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/libbytesize_unittest.py (1)
src/python/bytesize.py (3)
true_div(188-193)SizeStruct(84-241)new_from_str(94-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-10-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-ppc64le
- GitHub Check: rpm-build:centos-stream-10-ppc64le
- GitHub Check: rpm-build:centos-stream-9-ppc64le
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-42-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: csmock
🔇 Additional comments (6)
src/bs_size.c (1)
705-710: Good fix: proper cleanup on overflow error path.Adding
mpz_clear(max)before the error return prevents a memory leak when the size exceedsUINT64_MAX. This ensures the GMP variable is properly cleared on all exit paths.src/bs_size.h (2)
19-19: LGTM: New error code properly documented and added.The
BS_ERROR_FAILerror code is appropriately added to the end of the enum (ABI-compatible) and is used correctly inbs_size.cfor the memory allocation failure case inbs_size_get_bytes().Also applies to: 24-29
78-78: Documentation improvements look good.The comment clarifications and typo fixes improve code readability.
Also applies to: 85-86
tests/libbytesize_unittest.py (3)
436-446: LGTM: Comment typo fixes.The comment corrections from "accurancy" to "accuracy" improve documentation quality without affecting test behavior.
775-868: LGTM: Non-functional formatting changes.The whitespace and formatting adjustments in the docstring and test data are cosmetic and do not affect test logic.
891-897: LGTM: Comment formatting adjustment.Minor formatting change in the script entry point comments.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
vojtechtrefny
force-pushed
the
main_various-fixes
branch
from
3d75381 to
a1effe9
Compare
Mostly to make static analysis happy, but we'll also crash a bit more nicely than after the NULL dereference in bs_size_init.
vojtechtrefny
force-pushed
the
main_various-fixes
branch
from
a1effe9 to
26631d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/bs_size.c`:
- Around line 400-405: The bs_size_new() function uses assert(ret) which is
removed in release builds—replace this with an explicit NULL check after
malloc(sizeof(struct _BSSize)) in bs_size_new: if malloc returns NULL, handle it
like other allocations in this file (e.g., return NULL and optionally set errno
= ENOMEM or log an error) instead of asserting; keep the call to
bs_size_init(ret) only when ret is non-NULL so you don't dereference NULL and
maintain consistent NULL-return semantics used elsewhere.
♻️ Duplicate comments (2)
src/bs_size.c (2)
1216-1219: LGTM! Correctly returnsNULLfor pointer-returning function.Returning
NULLinstead of0for achar*return type is more idiomatic and consistent. This was already flagged and addressed in previous review.
1252-1254: LGTM! Correctly returnsNULLfor pointer-returning function.This change mirrors the fix in
bs_size_true_divand ensures consistent error handling for division-by-zero across both functions. This was already flagged and addressed in previous review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.fmf/versionis excluded by!.fmf/**dist/libbytesize.spec.inis excluded by!**/dist/**plans/blivet.fmfis excluded by!plans/**plans/tests.fmfis excluded by!plans/**
📒 Files selected for processing (5)
.coderabbit.yaml.packit.yamlsrc/bs_size.csrc/bs_size.htests/libbytesize_unittest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .packit.yaml
- .coderabbit.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
tests/libbytesize_unittest.py (1)
src/python/bytesize.py (3)
true_div(188-193)SizeStruct(84-241)new_from_str(94-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (44)
- GitHub Check: rpm-build:fedora-42-aarch64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-ppc64le
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:centos-stream-10-ppc64le
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:centos-stream-9-ppc64le
- GitHub Check: rpm-build:fedora-42-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-42-aarch64
- GitHub Check: rpm-build:centos-stream-10-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64:revdeps_blivet
- GitHub Check: testing-farm:fedora-43-aarch64
- GitHub Check: testing-farm:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-ppc64le
- GitHub Check: rpm-build:fedora-42-aarch64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-10-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-aarch64
- GitHub Check: rpm-build:fedora-43-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-ppc64le
- GitHub Check: rpm-build:fedora-43-aarch64
- GitHub Check: rpm-build:fedora-42-ppc64le
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: csmock
🔇 Additional comments (8)
src/bs_size.h (3)
19-19: LGTM! New error code for generic failures.The addition of
BS_ERROR_FAILprovides a generic error code for failures that don't fit other categories (e.g., memory allocation failures inbs_size_get_bytes). The placement at the end of the enum preserves existing error code values.Also applies to: 24-29
78-78: Documentation typo fix approved.Correctly changed "Generic unit fo size" to "Generic unit for size".
85-86: Documentation typo fix approved.The comment now correctly reads "numbers" instead of "numbets".
src/bs_size.c (2)
10-10: LGTM! Include for assert macro.Required for the assertion added in
bs_size_new.
705-710: Good fix: Memory leak prevented on overflow error path.Adding
mpz_clear(max)before returning ensures the GMP integer is properly freed when the size exceedsUINT64_MAX. This prevents a memory leak on the error path.tests/libbytesize_unittest.py (3)
439-439: LGTM! Typo corrections in comments.Fixed "accurancy" to "accuracy" in the
testTrueDivmethod comments.Also applies to: 444-444
775-783: LGTM! Minor formatting adjustments in test docstring and test data.Whitespace/blank line adjustments improve readability without affecting test logic.
Also applies to: 797-797, 860-860, 868-868
894-894: LGTM! Typo correction in comment.Fixed "intepret" to "interpret" in the
__main__guard comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| BSSize bs_size_new (void) { | ||
| BSSize ret = (BSSize) malloc (sizeof(struct _BSSize)); | ||
| assert (ret); | ||
| bs_size_init (ret); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the code at the specified location
cat -n src/bs_size.c | sed -n '395,410p'Repository: storaged-project/libbytesize
Length of output: 578
🏁 Script executed:
# Check for NDEBUG definition in build configuration
rg -n "NDEBUG" -g '*.am' -g '*.ac' -g 'configure*' -g 'Makefile*' -g 'meson.build' --max-count 20Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Check CFLAGS in configure.ac if it exists
if [ -f configure.ac ]; then
rg -n "CFLAGS" configure.ac
fiRepository: storaged-project/libbytesize
Length of output: 156
🏁 Script executed:
# Look for similar malloc patterns in the codebase to understand project conventions
rg -n "malloc.*assert" -A 2 -B 1 --type c | head -40Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Look for other error handling patterns in the same file
rg -n "abort\(\)|exit\(\)|malloc.*if|malloc.*NULL" src/bs_size.c -B 2 -A 2Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Check how other similar constructor/new functions handle allocation
ast-grep --pattern 'BSSize $_ (void) {
$$$
malloc($$$)
$$$
}' src/bs_size.cRepository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Look at other files in the codebase for common error handling patterns
rg -n "malloc.*assert|malloc.*if.*NULL|malloc.*abort" --type c | head -20Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Check if there's a CONTRIBUTING or design doc that explains error strategy
fd -type f \( -name "CONTRIBUTING*" -o -name "DESIGN*" -o -name "README*" -o -name "*.md" \) | head -10Repository: storaged-project/libbytesize
Length of output: 242
🏁 Script executed:
# Search for all malloc usage in bs_size.c to understand pattern consistency
rg -n "malloc" src/bs_size.c -B 1 -A 2Repository: storaged-project/libbytesize
Length of output: 695
🏁 Script executed:
# Check for documentation files
fd -type f \( -name "*.md" -o -name "README*" -o -name "DESIGN*" \)Repository: storaged-project/libbytesize
Length of output: 242
🏁 Script executed:
# Look at the full file to understand library style and conventions
wc -l src/bs_size.cRepository: storaged-project/libbytesize
Length of output: 90
🏁 Script executed:
# Check for glib dependencies (hint: libbytesize is a GLib-based library)
rg -n "g_malloc|g_error|GError" src/bs_size.c | head -10Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Check what other "new" functions do
rg -n "(\w+)_new.*\{" src/bs_size.c -A 5 | head -50Repository: storaged-project/libbytesize
Length of output: 1176
🏁 Script executed:
# Check how bs_size_new() is called from other functions - does it handle errors?
rg -n "bs_size_new\(" src/bs_size.c -B 1 -A 3Repository: storaged-project/libbytesize
Length of output: 54
🏁 Script executed:
# Try different pattern - look for calls to bs_size_new
rg -n "bs_size_new" src/bs_size.c -B 1 -A 3Repository: storaged-project/libbytesize
Length of output: 3352
🏁 Script executed:
# Also check if there are any other files that call bs_size_new
rg -n "bs_size_new" --type c --type hRepository: storaged-project/libbytesize
Length of output: 1418
Replace assert-based null check with explicit error handling that remains active in release builds.
The assert(ret) at line 402 becomes a no-op when NDEBUG is defined (standard in release builds). If malloc fails and returns NULL, line 403 will dereference a null pointer, causing undefined behavior. This is particularly problematic because bs_size_new() is called 17+ times throughout this file (lines 419, 648, 669, 882, 913, 942, 973, 1004, 1060, 1168, 1299, 1343) and none of these callers check for a NULL return. Additionally, the same file demonstrates proper NULL-check patterns for other malloc calls (e.g., lines 150–152, 194–196, 294–296), making the assert-based approach inconsistent with the project's own conventions.
🛠️ Suggested alternative
BSSize bs_size_new (void) {
BSSize ret = (BSSize) malloc (sizeof(struct _BSSize));
- assert (ret);
+ if (!ret) {
+ abort();
+ }
bs_size_init (ret);
return ret;
}🤖 Prompt for AI Agents
In `@src/bs_size.c` around lines 400 - 405, The bs_size_new() function uses
assert(ret) which is removed in release builds—replace this with an explicit
NULL check after malloc(sizeof(struct _BSSize)) in bs_size_new: if malloc
returns NULL, handle it like other allocations in this file (e.g., return NULL
and optionally set errno = ENOMEM or log an error) instead of asserting; keep
the call to bs_size_init(ret) only when ret is non-NULL so you don't dereference
NULL and maintain consistent NULL-return semantics used elsewhere.
2 participants
Packit is here mostly to run Blivet tests which is the primary user of libbytesize and has quite a lot size-related test cases.
Summary by CodeRabbit
New Features
CI / Chores
Bug Fixes
Style / Docs
✏️ Tip: You can customize this high-level summary in your review settings.