Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
language: "en-US"
early_access: false
reviews:
profile: "chill"
poem: false
in_progress_fortune: false
sequence_diagrams: false
related_issues: false
related_prs: false
suggested_labels: false
suggested_reviewers: false
path_filters: ["!po/**", "!plans/**", "!.github/**", "!.fmf/**"]
chat:
art: false
1 change: 1 addition & 0 deletions .fmf/version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
1
29 changes: 28 additions & 1 deletion .packit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,41 @@ jobs:
dist_git_branches:
- fedora-branched

- job: tests
trigger: pull_request
targets:
- fedora-latest-x86_64
- fedora-latest-aarch64
- fedora-latest-stable-x86_64
- fedora-latest-stable-aarch64
branch: main

# run tests for libbytesize consumers, see plans/ with `revdeps_blivet == yes`
- job: tests
identifier: revdeps_blivet
trigger: pull_request
notifications:
failure_comment:
message: "Blivet tests failed for commit {commit_sha}. @vojtechtrefny please check."
targets:
- fedora-latest-stable
branch: main
tf_extra_params:
environments:
- artifacts:
- type: repository-file
id: https://copr.fedorainfracloud.org/coprs/g/storage/blivet-daily/repo/fedora-$releasever/group_storage-blivet-daily-fedora-$releasever.repo
tmt:
context:
revdeps_blivet: "yes"

srpm_build_deps:
- make
- gcc
- libtool
- autoconf
- automake
- gmp-devel
- mpfr-devel
- pcre2-devel
- gettext-devel
- python3-devel
Expand Down
1 change: 0 additions & 1 deletion dist/libbytesize.spec.in
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ Source0: https://github.com/storaged-project/libbytesize/releases/download/%
BuildRequires: make
BuildRequires: gcc
BuildRequires: gmp-devel
BuildRequires: mpfr-devel
BuildRequires: pcre2-devel
BuildRequires: gettext-devel
%if %{with_python3}
Expand Down
35 changes: 35 additions & 0 deletions plans/blivet.fmf
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# reverse dependency test for blivet
enabled: false

adjust+:
when: revdeps_blivet == yes
enabled: true

prepare:
- name: copr
how: shell
script:
- sudo dnf install -y python3-libdnf5 'dnf-command(copr)'
- sudo dnf copr enable -y @storage/udisks-daily
# TF prioritizes Fedora tag repo over all others, in particular our daily COPR
- for f in $(grep -l -r 'testing-farm-tag-repository' /etc/yum.repos.d); do sed -i '/priority/d' "$f" ;done
- sudo dnf -y update

- name: ansible
how: shell
script:
- sudo dnf install -y curl ansible
- curl -Ok https://raw.githubusercontent.com/storaged-project/blivet/main/misc/install-test-dependencies.yml
- curl -Ok https://raw.githubusercontent.com/storaged-project/blivet/main/misc/blivet-tasks.yml
- ansible-playbook -K -i "localhost," -c local install-test-dependencies.yml

discover:
how: shell
url: https://github.com/storaged-project/blivet
ref: main
tests:
- name: all
test: make test

execute:
how: tmt
14 changes: 14 additions & 0 deletions plans/tests.fmf
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
summary: Run tests

adjust+:
- when: revdeps_blivet == yes
enabled: false

prepare:
- name: ansible
how: ansible
playbook: misc/install-test-dependencies.yml

execute:
how: tmt
script: ./autogen.sh && ./configure && make -j && make ci
7 changes: 5 additions & 2 deletions src/bs_size.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <string.h>
#include <ctype.h>
#include <limits.h>
#include <assert.h>

/* set code unit width to 8 so we can use generic macros like 'pcre2_compile'
* instead of 'pcre2_compile_8'
Expand Down Expand Up @@ -398,6 +399,7 @@ void bs_clear_error (BSError **error) {
*/
BSSize bs_size_new (void) {
BSSize ret = (BSSize) malloc (sizeof(struct _BSSize));
assert (ret);
bs_size_init (ret);
return ret;
}
Comment on lines 400 to 405
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 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 20

Repository: 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
fi

Repository: 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 -40

Repository: 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 2

Repository: 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.c

Repository: 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 -20

Repository: 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 -10

Repository: 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 2

Repository: 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.c

Repository: 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 -10

Repository: 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 -50

Repository: 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 3

Repository: 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 3

Repository: 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 h

Repository: 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.

Expand Down Expand Up @@ -701,6 +703,7 @@ uint64_t bs_size_get_bytes (const BSSize size, int *sgn, BSError **error) {
mpz_set_str (max, num_str, 10);
free (num_str);
if (mpz_cmp (size->bytes, max) > 0) {
mpz_clear (max);
set_error (error, BS_ERROR_OVER, strdup("The size is too big, cannot be returned as a 64bit number of bytes"));
return 0;
}
Expand Down Expand Up @@ -1212,7 +1215,7 @@ char* bs_size_true_div (const BSSize size1, const BSSize size2, BSError **error)

if (mpz_cmp_ui (size2->bytes, 0) == 0) {
set_error (error, BS_ERROR_ZERO_DIV, strdup_printf("Division by zero"));
return 0;
return NULL;
}

mpf_init2 (op1, BS_FLOAT_PREC_BITS);
Expand Down Expand Up @@ -1248,7 +1251,7 @@ char* bs_size_true_div_int (const BSSize size, uint64_t divisor, BSError **error

if (divisor == 0) {
set_error (error, BS_ERROR_ZERO_DIV, strdup_printf ("Division by zero"));
return 0;
return NULL;
} else if (divisor > ULONG_MAX) {
set_error (error, BS_ERROR_OVER, strdup_printf ("Divisor too big, must be less or equal to %lu", ULONG_MAX));
return NULL;
Expand Down
5 changes: 3 additions & 2 deletions src/bs_size.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ typedef struct _BSSize * BSSize;
* @BS_ERROR_INVALID_SPEC: invalid size or unit spec provided
* @BS_ERROR_OVER: a value is over the limits imposed by a type
* @BS_ERROR_ZERO_DIV: an attempt to do division by zero
* @BS_ERROR_FAIL: generic failure error code
*
* Error codes that identify various errors that can occur while working with
* #BSSize instances.
Expand Down Expand Up @@ -74,14 +75,14 @@ typedef enum {
* @bunit: a binary unit
* @dunit: a decimal unit
*
* Generic unit fo size in bytes.
* Generic unit for size in bytes.
*/
typedef union {
BSBunit bunit;
BSDunit dunit;
} BSUnit;

/* use 256 bits of precision for floating point numbets, that should be more
/* use 256 bits of precision for floating point numbers, that should be more
than enough */
/**
* BS_FLOAT_PREC_BITS:
Expand Down
16 changes: 8 additions & 8 deletions tests/libbytesize_unittest.py
Original file line number Diff line number Diff line change
Expand Up @@ -436,12 +436,12 @@ def testSgn(self):
def testTrueDiv(self):
x = SizeStruct.new_from_str("1024 B")
y = SizeStruct.new_from_str("-102.4 B") # rounds to whole bytes
divResult = float(x.true_div(y)[:15].replace(locale.nl_langinfo(locale.RADIXCHAR), ".")) # just some number to cover accurancy and not cross max float range
divResult = float(x.true_div(y)[:15].replace(locale.nl_langinfo(locale.RADIXCHAR), ".")) # just some number to cover accuracy and not cross max float range
self.assertAlmostEqual(divResult, 1024.0/-102.0)

x = SizeStruct.new_from_str("1 MiB")
y = SizeStruct.new_from_str("1 KiB")
divResult = float(x.true_div(y)[:15].replace(locale.nl_langinfo(locale.RADIXCHAR), ".")) # just some number to cover accurancy and not cross max float range
divResult = float(x.true_div(y)[:15].replace(locale.nl_langinfo(locale.RADIXCHAR), ".")) # just some number to cover accuracy and not cross max float range
self.assertAlmostEqual(divResult, 1024.0)
#enddef

Expand Down Expand Up @@ -774,8 +774,8 @@ def testTrueDivInt(self):
@requires_locales({'en_US.UTF-8'})
def testPowerComputationRoundingIssues(self):
"""Test cases that expose rounding differences when using floating-point arithmetic.
These test cases were discovered by fuzzing and demonstrate that both GMP and MPFR

These test cases were discovered by fuzzing and demonstrate that both GMP and MPFR
can produce incorrect results due to floating-point rounding errors: half of these
fail using GMP floating-point arithmetic, and half fail using MPFR floating-point
arithmetic. They all pass using rational arithmetic, so this can be considered a
Expand All @@ -794,7 +794,7 @@ def testPowerComputationRoundingIssues(self):
('0.0042875429 EB', 4287542900000000),
('0.0324645967 YB', 32464596700000000000000),
('0.1417885628 ZB', 141788562800000000000),

# Medium values
('1.2558302853 TB', 1255830285300),
('1.2808632839 TB', 1280863283900),
Expand Down Expand Up @@ -857,15 +857,15 @@ def testPowerComputationRoundingIssues(self):
('128.1037376252 PB', 128103737625200000),
('130.4743561323 ZB', 130474356132300000000000),
('138.2867513494 YB', 138286751349400000000000000),

# Large values
('18258.0630890156 PB', 18258063089015600000),
('18800.1176214700 EB', 18800117621470000000000),
('269636.0318459886 YB', 269636031845988600000000000000),
('276686.6833125990 YB', 276686683312599000000000000000),
('535817.4105711933 EB', 535817410571193300000000),
]

for test_str, expected_bytes in test_cases:
with self.subTest(test_str=test_str):
size = SizeStruct.new_from_str(test_str)
Expand All @@ -891,7 +891,7 @@ def testPowerComputationRoundingIssues(self):
if __name__=='__main__':
if len(sys.argv) > 1:
DEFAULT_LOCALE = sys.argv[1]
# the unittest module would try to intepret the argument too, let's
# the unittest module would try to interpret the argument too, let's
# remove it
sys.argv = [sys.argv[0]]
unittest.main()
Expand Down