Code review notes, pass 2: identity hot-plug, GPT, USB, acceptance scripts
Findings reported, not fixed (Captain Bob's Law). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017RzZgrf7V7Zk2iczBXDRbm
This commit is contained in:
@@ -63,7 +63,65 @@ reachable at MESH steps 8–9. A test written first (Bob's rule) would show it.
|
||||
(STICK.md 6 order); line editor's over-long line refused whole (not truncated and run); logout ends a
|
||||
waiting line (Bob 2026-10-26); size message buffer sized right (28 bytes incl. NUL).
|
||||
|
||||
## kernel v4 identity, the rest (identity.c 320–657) — hot-plug, offer, fixed screen
|
||||
- **L — `sk_v4_idle` reallocates `g_usb[xc].slots` when `count < max_slots + 1` without copying or freeing the
|
||||
old array.** `g_offered`, `g_spare`, `g_pending_real`, `g_real_drive` and `g_zuse_dev` (= `&d->part` inside
|
||||
the old array) would then point into the abandoned array. Only reachable if a controller's `max_slots` grows
|
||||
after the first allocation, which it does not today — latent, plus a leak.
|
||||
- **L — partial state on a failed identity attach:** in `sk_v4_drive_arrived`, Artemis is attached and
|
||||
`g_stick_in = 1` before the identity partition's `blk_subsys_attach_device`; if that fails the function
|
||||
returns with Artemis attached and no identity, and `d->used` unset. A later pull detaches Artemis correctly
|
||||
(`art_used`), so it is recoverable; the message names xhci, not the partition.
|
||||
- **Ruled, not a finding:** the fixed screen lets anyone holding the system stick press R and mint a
|
||||
replacement real Zuse (STICK.md §6a, Bob 2026-10-29: "the old stick dies for good"; copying a system stick
|
||||
is "not guarded against", §6a line 252).
|
||||
- OK: marker cleared when the system stick leaves; a real-Zuse stick seen before the system stick is held
|
||||
(`g_pending_real`) and judged when it arrives; the offered stick is never the system stick
|
||||
(`sk_v4_is_this_stick`); waits use `arch_halt()` (#151).
|
||||
|
||||
## kernel/src/gpt.c
|
||||
- Sound overall: header CRC, entry-array CRC, `my_lba == 1`, entries inside the usable range, entry array before
|
||||
first_usable and 1 KiB-aligned; writes go backup first, then primary, then the protective MBR last, with
|
||||
flushes between, so a power cut leaves one valid table. `gpt_write_one` zeroes the MBR first.
|
||||
- **L — `gpt_read` stops validating at `max` entries:** `for (i < nent && n < max)` — with `max = 4`
|
||||
(`sk_v4_mint_zuse`) or `SK_V4_GPT_PARTS`, a disk whose identity/Artemis entry is past the first `max` used
|
||||
entries reads as having none, and the entries after `max` are not range-checked. Harmless for this
|
||||
system's own sticks (one or two partitions); worth knowing for foreign disks.
|
||||
- **L — no backup-header fallback:** a damaged primary GPT returns GPT_BAD ("damaged partition table -- not
|
||||
used") though the backup may be good. A choice, not a bug; the message is honest.
|
||||
- **L — 32-bit fblock arithmetic** (`(uint32_t)(ent_lba / 2u) + i`, `st->first_fblock`): bounded by
|
||||
`total_blocks` (uint32 of 1 KiB = 4 TiB), consistent with the block layer.
|
||||
|
||||
## kernel/src/usb (xhci.c descriptor walk, blkio_usb.c, usb_kbd.c)
|
||||
- OK: device-controlled lengths are clamped — `xhci_ep0_get_config_descriptor` clamps the request to the
|
||||
128-byte buffer, every descriptor walk checks `off + field < len` and breaks on `bLength == 0`; HID packet
|
||||
capped at 64; keyboard report capped at 8 bytes, usage lookup bounds-checked; SCSI block size must divide
|
||||
1 KiB, capacity saturates at 2^32 Forth blocks.
|
||||
- **L/M — a device whose `wTotalLength <= 9` loops enumeration:** the second GET_DESCRIPTOR is again classed
|
||||
`XHCI_XFER_CONFIG_DESC_SHORT` (`length <= 9`), whose handler asks again for `total_len` — for ever, for that
|
||||
slot. A hostile or broken device only; `wTotalLength = 0` also issues a zero-length data stage.
|
||||
- **L — config descriptors longer than 128 bytes are cut at 128.** Interfaces past byte 128 are never seen.
|
||||
BOT is normally the first interface and a boot keyboard is small, so QEMU never shows it; a composite
|
||||
device on real hardware (SER5 bring-up) could. No message says the descriptor was cut.
|
||||
|
||||
## scripts/nightly.sh, scripts/qemu_session.py, .gitea/workflows/acceptance.yml
|
||||
- OK: `set -euo pipefail`, so a failing `qemu_session.py` or `make test-32` behind `| tee` fails the stage;
|
||||
`POST: PASSED` is printed by `v4/system/boot.c` only when `v4_post_run` reported no failures, after the
|
||||
`PARITY:V4_SYSTEM` line; one QEMU at a time via the shared `qemu` concurrency group;
|
||||
`git checkout -- disk/zuse-scratch` before each architecture.
|
||||
- **L — the dict_hash agreement check is two-of-three tolerant:** the stick-high stage collects hashes with
|
||||
`sort -u` and passes if exactly one distinct value appears. An architecture whose log has no
|
||||
`PARITY:V4_SYSTEM` line (or whose `LOG:` line is missing) contributes nothing and the check still passes.
|
||||
Today each arch's own stage would have failed first (no POST: PASSED), so it is belt without braces: count
|
||||
three hashes, not one distinct value.
|
||||
- **L — the POST test count is not asserted.** The harness checks `POST: PASSED` (and `fail=0` only in the
|
||||
`stick_v3` mode); a POST that lost cases would still pass. CLAUDE.md says "tests=538 (the count grows)", so
|
||||
this is policy, but a floor would catch a silently shrinking POST.
|
||||
- **L — acceptance without `CAPSULE_SIGN_KEY` runs unsigned with a warning, and passes.** Fine for a fork;
|
||||
for the gate into `master` an unsigned pass may not be what is meant.
|
||||
|
||||
## Not yet reviewed (honest)
|
||||
identity.c 320–657 (hot-plug, offered stick, beat), gpt.c, usb/xhci.c, usb_kbd.c, hal, boot loader, the rest of
|
||||
kernel/src (kernel_main.c 1986 lines, repl.c — v3 path), v4/system/post_cases.c, text.c, image.c, v4/tools,
|
||||
capsules/v4, tools/mkcapsule.c, scripts/nightly.sh + qemu_session.py, .gitea/workflows, Makefiles, v3.
|
||||
kernel_main.c (1986 lines) and repl.c (the v3 path), hal, the UEFI loader, virtio, the block subsystem
|
||||
(`block_subsystem.c`), capsule_zuse_boot / homeblocks_sig / artemis_sig, v4/system/post_cases.c, text.c,
|
||||
image.c, v4/tools (mkrom, mkmap, hosted), capsules/v4/*.4th, tools/mkcapsule.c beyond the #143 warnings,
|
||||
qemu_session.py modes other than zuse/stick_genesis, build/nightly/release workflows, Makefiles, v3.
|
||||
|
||||
Reference in New Issue
Block a user