[wic][PATCH 0/5] ksparser: four argparse-type fixes plus unit coverage

[wic][PATCH 0/5] ksparser: four argparse-type fixes plus unit coverage

[Trevor Woerner]

This series continues the standalone unit-test work, this time over the
custom argparse types in ksparser.py: sizetype(), overheadtype(), and
systemidtype(). It keeps the shape the suite settled on: each source fix
is its own standalone commit, and the green test module lands last, so
the suite passes at every commit.

Four fixes come first, each independent of the others:

  - sizetype() accepted a leading minus, so "-1M" returned -1024 and
    "-5" returned -5120. A negative size is meaningless for every
    consumer (--size, --fixed-size, --offset, --extra-*-space, the empty
    source plugin's size=/bs=); reject it.
  - overheadtype()'s below-1.0 error path did "...message..." % arg with
    no %-placeholder, so every out-of-range value crashed with an opaque
    TypeError instead of the intended ArgumentTypeError. Add the missing
    placeholder.
  - overheadtype() let float('nan') and float('inf') slip past the < 1.0
    guard, which then multiplied the rootfs size by nan/inf downstream.
    Reject any non-finite factor up front.
  - systemidtype() converted with int(arg, 16), which also accepts a
    bare "82" (hex 130) and a signed "+0x82", contradicting its
    documented "hex between 0x1 and 0xFF" contract. Require an explicit
    0x prefix.

The final commit adds tests/unit/test_ksparser_types.py, covering the
three types' accepted forms, boundaries, and rejected inputs, and
locking in the four fixes. Backing any fix out turns the matching test
red. The suite is green at every commit and ruff-clean.

Trevor Woerner (5):
  ksparser: reject negative sizes in sizetype()
  ksparser: fix crash on out-of-range overhead factor
  ksparser: reject non-finite overhead factors
  ksparser: require an explicit 0x prefix for --system-id
  tests/unit/test_ksparser_types: cover ksparser's argparse types

 src/wic/ksparser.py               |  15 +-
 tests/unit/test_ksparser_types.py | 226 ++++++++++++++++++++++++++++++
 2 files changed, 236 insertions(+), 5 deletions(-)
 create mode 100644 tests/unit/test_ksparser_types.py

-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 1/5] ksparser: reject negative sizes in sizetype()

[Trevor Woerner]

sizetype() splits "<num>[S|s|K|k|M|G]" into an integer count and a unit
suffix, then multiplies. int() happily parses a leading minus, so "-1M"
produced -1024 and "-5" produced -5120 with no complaint. A negative
size is meaningless for every consumer of sizetype() (--size,
--fixed-size, --offset, --extra-*-space, and the empty source plugin's
size=/bs= params); it can only corrupt the resulting image layout.

Reject a negative size with the same ArgumentTypeError the parser already
raises for other malformed sizes.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/ksparser.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/wic/ksparser.py b/src/wic/ksparser.py
index 8e5fbfadd81f..24c7015b336d 100644
--- a/src/wic/ksparser.py
+++ b/src/wic/ksparser.py
@@ -68,6 +68,8 @@ def sizetype(default, size_in_bytes=False):
             except ValueError:
                 raise ArgumentTypeError("Invalid size: %r" % arg)
 
+        if size < 0:
+            raise ArgumentTypeError("Invalid size: %r" % arg)
 
         if size_in_bytes:
             if suffix == 's' or suffix == 'S':
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 2/5] ksparser: fix crash on out-of-range overhead factor

[Trevor Woerner]

overheadtype() rejects a factor below 1.0, but the error path read

    raise ArgumentTypeError("Overhead factor should be > 1.0" % arg)

The message string carries no %-placeholder, so the `% arg`
interpolation itself raised TypeError ("not all arguments converted
during string formatting") before the ArgumentTypeError could be
constructed. Every out-of-range value (0.0, 0.5, -1.0, -inf) crashed
with an opaque TypeError instead of the intended argument error.

Add the missing placeholder so the offending value is reported and the
correct ArgumentTypeError is raised.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/ksparser.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/wic/ksparser.py b/src/wic/ksparser.py
index 24c7015b336d..a73ae56cd196 100644
--- a/src/wic/ksparser.py
+++ b/src/wic/ksparser.py
@@ -99,7 +99,7 @@ def overheadtype(arg):
         raise ArgumentTypeError("Invalid value: %r" % arg)
 
     if result < 1.0:
-        raise ArgumentTypeError("Overhead factor should be > 1.0" % arg)
+        raise ArgumentTypeError("Overhead factor should be > 1.0, not %r" % arg)
 
     return result
 
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 3/5] ksparser: reject non-finite overhead factors

[Trevor Woerner]

overheadtype() guards against factors below 1.0, but float("nan") and
float("inf") slip past that check: nan compares false against every
bound, and inf is trivially greater than 1.0. wic then multiplied the
rootfs size by nan or inf (partition.py does
int(rootfs_size * self.overhead_factor)), producing a ValueError deep in
image sizing or a nonsensical partition size.

Reject any non-finite factor up front with math.isfinite() so the error
is reported at parse time alongside the other overhead validation.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/ksparser.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/wic/ksparser.py b/src/wic/ksparser.py
index a73ae56cd196..528a8180826f 100644
--- a/src/wic/ksparser.py
+++ b/src/wic/ksparser.py
@@ -15,6 +15,7 @@
 import os
 import shlex
 import logging
+import math
 import re
 import uuid
 
@@ -98,7 +99,7 @@ def overheadtype(arg):
     except ValueError:
         raise ArgumentTypeError("Invalid value: %r" % arg)
 
-    if result < 1.0:
+    if not math.isfinite(result) or result < 1.0:
         raise ArgumentTypeError("Overhead factor should be > 1.0, not %r" % arg)
 
     return result
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 4/5] ksparser: require an explicit 0x prefix for --system-id

[Trevor Woerner]

systemidtype() validates the MBR partition type as "hex between 0x1 and
0xFF", but it converts with int(arg, 16), which also accepts a bare "82"
(parsed as hex 0x82 = 130) and a signed form like "+0x82". Those inputs
contradict the documented contract and are silently returned unchanged,
so the raw string later handed to sfdisk may not be what the user meant.

Require an explicit 0x/0X-prefixed hex string before converting, so only
the documented form is accepted and every other spelling raises the
existing ArgumentTypeError.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 src/wic/ksparser.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/wic/ksparser.py b/src/wic/ksparser.py
index 528a8180826f..dcf26f788083 100644
--- a/src/wic/ksparser.py
+++ b/src/wic/ksparser.py
@@ -132,10 +132,12 @@ def systemidtype(arg):
     """
     error = "Invalid system type: %s. must be hex "\
             "between 0x1 and 0xFF" % arg
-    try:
-        result = int(arg, 16)
-    except ValueError:
+    # int(arg, 16) also accepts a bare "82" (-> 130) and a leading sign
+    # like "+0x82"; the documented contract is an explicit 0x-prefixed
+    # hex byte, so require that form before converting.
+    if not re.fullmatch(r"0[xX][0-9a-fA-F]+", arg or ""):
         raise ArgumentTypeError(error)
+    result = int(arg, 16)
 
     if result <= 0 or result > 0xff:
         raise ArgumentTypeError(error)
-- 
2.50.0.173.g8b6f19ccfc3a

[wic][PATCH 5/5] tests/unit/test_ksparser_types: cover ksparser's argparse types

[Trevor Woerner]

Add direct unit coverage for sizetype(), overheadtype(), and
systemidtype(): the accepted KiB/byte-mode units and default-suffix
handling, the >= 1.0 overhead contract, and the 0x-prefixed system-id
byte range, together with the boundary and malformed inputs each
rejects.

The cases pin behaviour that is easy to regress: sizetype() rejects a
negative count, overheadtype() raises a clean ArgumentTypeError (not a
TypeError) for a sub-1.0 factor and refuses nan/inf, and systemidtype()
accepts only an explicit 0x-prefixed hex byte.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
 tests/unit/test_ksparser_types.py | 226 ++++++++++++++++++++++++++++++
 1 file changed, 226 insertions(+)
 create mode 100644 tests/unit/test_ksparser_types.py

diff --git a/tests/unit/test_ksparser_types.py b/tests/unit/test_ksparser_types.py
new file mode 100644
index 000000000000..49ac38ba5397
--- /dev/null
+++ b/tests/unit/test_ksparser_types.py
@@ -0,0 +1,226 @@
+"""
+Unit tests for wic/ksparser.py's custom argparse types: sizetype,
+overheadtype, and systemidtype. These are wic's own input validators for
+kickstart/command-line values, so the tests pin the accepted forms, the
+rejected forms, and the boundaries between them.
+"""
+import sys
+from pathlib import Path
+
+import pytest
+
+_SRC = Path(__file__).resolve().parent.parent.parent / "src"
+if str(_SRC) not in sys.path:
+    sys.path.insert(0, str(_SRC))
+
+from argparse import ArgumentTypeError
+
+from wic.ksparser import sizetype, overheadtype, systemidtype
+
+
+class TestSizetype:
+    """sizetype(default, size_in_bytes=False) returns f(arg) that turns a
+    "<num>[S|s|K|k|M|G]" string into an integer. With size_in_bytes=False
+    the multiplier is 1 (K->1, M->1024, G->1024*1024, i.e. KiB counts);
+    with size_in_bytes=True the multiplier is 1024 and S/s means sectors of
+    512 bytes. A bare number takes the default suffix."""
+
+    @pytest.mark.parametrize("arg,expected", [
+        ("0", 0),
+        ("0M", 0),
+        ("1K", 1),
+        ("1k", 1),
+        ("3K", 3),
+        ("7K", 7),
+        ("512k", 512),
+        ("999k", 999),
+        ("1024K", 1024),
+        ("1000K", 1000),
+        ("1M", 1 * 1024),
+        ("10M", 10 * 1024),
+        ("13M", 13 * 1024),
+        ("100M", 100 * 1024),
+        ("512M", 512 * 1024),
+        ("1G", 1 * 1024 * 1024),
+        ("2G", 2 * 1024 * 1024),
+        ("3G", 3 * 1024 * 1024),
+        # a large magnitude past 2**31, to catch any 32-bit truncation
+        ("5000000K", 5000000),
+    ])
+    def test_kib_units_with_M_default(self, arg, expected):
+        assert sizetype("M")(arg) == expected
+
+    def test_bare_number_uses_M_default(self):
+        assert sizetype("M")("100") == 100 * 1024
+
+    def test_bare_number_uses_K_default(self):
+        assert sizetype("K")("512") == 512
+
+    @pytest.mark.parametrize("arg,expected", [
+        ("1S", 512),
+        ("1s", 512),
+        ("2S", 2 * 512),
+        ("7S", 7 * 512),
+        ("1K", 1024),
+        ("3K", 3 * 1024),
+        ("17M", 17 * 1024 * 1024),
+        ("1M", 1024 * 1024),
+        ("1G", 1024 ** 3),
+        # past 2**63 to confirm Python's arbitrary-precision ints hold
+        ("10000000G", 10000000 * 1024 ** 3),
+    ])
+    def test_size_in_bytes_units(self, arg, expected):
+        assert sizetype("K", size_in_bytes=True)(arg) == expected
+
+    def test_sectors_only_when_size_in_bytes(self):
+        # 'S' is a valid suffix only in byte mode; without it the suffix is
+        # unknown and rejected.
+        with pytest.raises(ArgumentTypeError):
+            sizetype("M")("1S")
+
+    # --- suffix case sensitivity -------------------------------------------
+
+    @pytest.mark.parametrize("arg", ["1m", "1g"])
+    def test_lowercase_m_and_g_rejected(self, arg):
+        # Only k/K are case-insensitive; m and g are not accepted.
+        with pytest.raises(ArgumentTypeError):
+            sizetype("M")(arg)
+
+    # --- negative sizes -----------------------------------------------------
+
+    @pytest.mark.parametrize("arg", ["-1M", "-5", "-1K", "-1024K", "-1G"])
+    def test_negative_size_rejected(self, arg):
+        with pytest.raises(ArgumentTypeError):
+            sizetype("M")(arg)
+
+    # --- invalid formats ----------------------------------------------------
+
+    @pytest.mark.parametrize("bad", [
+        "abc",       # no digits
+        "M",         # suffix only
+        "1X",        # unsupported suffix
+        "1.5M",      # float not accepted
+        "1M ",       # trailing space becomes the suffix
+        "1e3M",      # scientific notation
+        "",          # empty
+        "   ",       # whitespace only
+        "1;M",       # shell metacharacters
+        "1|M",
+        "$(1)M",
+        "1`M",
+    ])
+    def test_invalid_format_rejected(self, bad):
+        with pytest.raises(ArgumentTypeError):
+            sizetype("M")(bad)
+
+    def test_none_rejected(self):
+        with pytest.raises((ArgumentTypeError, TypeError)):
+            sizetype("M")(None)
+
+
+class TestOverheadtype:
+    """overheadtype(arg) -> float, and the factor must be >= 1.0. Anything
+    below 1.0, non-numeric, or non-finite is an ArgumentTypeError."""
+
+    @pytest.mark.parametrize("arg,expected", [
+        ("1.0", 1.0),
+        ("1.001", 1.001),
+        ("1.1", 1.1),
+        ("1.3", 1.3),
+        ("1.37", 1.37),
+        ("3.14159", 3.14159),
+        ("7", 7.0),
+        ("2", 2.0),
+        ("2.0", 2.0),
+        ("100.0", 100.0),
+        ("1000000.0", 1000000.0),
+    ])
+    def test_valid_factor(self, arg, expected):
+        assert overheadtype(arg) == pytest.approx(expected)
+
+    def test_leading_whitespace_accepted(self):
+        # float() strips surrounding whitespace.
+        assert overheadtype(" 1.1") == pytest.approx(1.1)
+
+    def test_trailing_whitespace_accepted(self):
+        assert overheadtype("1.1 ") == pytest.approx(1.1)
+
+    # --- below 1.0: a clean ArgumentTypeError, not a TypeError crash --------
+
+    @pytest.mark.parametrize("arg", ["0.0", "0.5", "0.999", "-1.0", "-0.5"])
+    def test_below_one_rejected(self, arg):
+        with pytest.raises(ArgumentTypeError):
+            overheadtype(arg)
+
+    # --- non-finite ---------------------------------------------------------
+
+    @pytest.mark.parametrize("arg", ["nan", "NaN", "inf", "Infinity", "-inf"])
+    def test_non_finite_rejected(self, arg):
+        with pytest.raises(ArgumentTypeError):
+            overheadtype(arg)
+
+    # --- non-numeric --------------------------------------------------------
+
+    @pytest.mark.parametrize("bad", [
+        "abc", "1.1.1", "M", "1,3", "1 1", "--1.1", "",
+    ])
+    def test_non_numeric_rejected(self, bad):
+        with pytest.raises(ArgumentTypeError):
+            overheadtype(bad)
+
+    def test_none_rejected(self):
+        with pytest.raises((ArgumentTypeError, TypeError)):
+            overheadtype(None)
+
+
+class TestSystemidtype:
+    """systemidtype(arg) validates an MBR partition type id as an explicit
+    0x-prefixed hex byte in 0x1..0xFF and returns the original string
+    unchanged."""
+
+    @pytest.mark.parametrize("arg", [
+        "0x1", "0x01", "0x0b", "0x0B", "0x82", "0x83", "0xFE", "0xFF",
+        "0xff", "0xFf", "0X82",
+        # odd, non-round bytes across the range
+        "0x7", "0x2a", "0x5b", "0x93", "0xa7", "0xd3",
+    ])
+    def test_valid_returned_unchanged(self, arg):
+        assert systemidtype(arg) == arg
+
+    # --- out of range -------------------------------------------------------
+
+    @pytest.mark.parametrize("arg", ["0x0", "0x00", "0x100", "0xFFFF"])
+    def test_out_of_range_rejected(self, arg):
+        with pytest.raises(ArgumentTypeError):
+            systemidtype(arg)
+
+    # --- must be an explicit 0x-prefixed hex string -------------------------
+
+    @pytest.mark.parametrize("arg", [
+        "82",        # no 0x prefix (would parse as hex 130)
+        "130",       # decimal that names a valid value
+        "+0x82",     # leading sign
+        "-0x82",
+        "0b1010",    # binary prefix
+    ])
+    def test_non_prefixed_forms_rejected(self, arg):
+        with pytest.raises(ArgumentTypeError):
+            systemidtype(arg)
+
+    @pytest.mark.parametrize("bad", [
+        "0x",              # prefix with no digit
+        "0xGG",            # invalid hex digit
+        "abc",             # not hex
+        "0x 82",           # embedded whitespace
+        " 0x82",           # leading whitespace
+        "",                # empty
+        "   ",             # whitespace only
+        "0x" + "F" * 10000,  # very long
+    ])
+    def test_invalid_format_rejected(self, bad):
+        with pytest.raises(ArgumentTypeError):
+            systemidtype(bad)
+
+    def test_none_rejected(self):
+        with pytest.raises((ArgumentTypeError, TypeError)):
+            systemidtype(None)
-- 
2.50.0.173.g8b6f19ccfc3a