[GH-ISSUE #36] sysinstall/instmedia: deferred minor review findings #29

New issue

Open

opened 2026-11-03 22:02:34 -07:00 by gitea-mirror · 1 comment

gitea-mirror commented

2026-11-03 22:02:34 -07:00

Owner

Originally created by @raynorpat on GitHub (Oct 6, 2026).
Original GitHub issue: https://github.com/RhapsodiOS/RhapsodiOS/issues/36

Minor findings the phase 5a reviews deferred (installer merged in 197a04b60; design docs/superpowers/specs/2026-10-05-sysinstall-design.md, record docs/build/sysinstall.md). None blocks use.

Disk layout (layout.c)

Missing edge tests: the 16- and 64-head bands, the total > 63*255*1024 branch, CHS at cylinder 1023 vs 1024, decoding a garbage signed MBR.
An active byte other than 0x80 is normalised to inactive on re-encode.
layout_check doesn't refuse disks under 1 GB itself; only layout_auto does.
An entry at LBA 0 or over the MBR sector isn't rejected.
test_check_rules asserts only non-NULL, not which rule fired.

Sets and config (sets.c, config.c)

A title line after the first package line is read as a package named "title".
A package line with embedded whitespace is accepted as one name.
An over-long title or description rejects the whole file (untested).
The password hash isn't checked for : or newline (crypt output can't contain them).
Missing tests: a root line without a trailing newline, a CRLF master.passwd, make_salt determinism, one set naming a package twice.

driverDetect

Auto Detect IDs values of 1024 bytes or more are skipped silently.
The table parser stops at the first malformed entry.
A table with no Location key gets a second Location appended (untested).
Exits 0 with an empty list when /usr/Devices is missing; sysinstall can't tell that from "no cards".
src/driverkit-3/PB.project doesn't list driverDetect.
An unchecked malloc for table names; t2 leaks if spew fails in writeRoot.
The Makefile omits tests/ from SOURCEFILES and hardcodes i386 paths; the ppc slice was never compiled.
The full PCI scan costs up to 256 buses × 32 devices of calls; match order follows readdir order.
No host tests for replaceDisk, rootDisk, or the Boot/Active composition in -w.

Write phase and screens (steps.c, main.c, ui.c, disks.c)

The pclose status of the ESP stream is ignored, so a gzip failure reads as a size error.
master.passwd's mode relies on the apk shipping 0600; an explicit chmod would be safer.
The step 6 parent directories must exist from the apks; otherwise the error is generic.
Missing tests: a missing or short boot0, the disk-name bound.
int blocks in the DKIOCNUMBLKS call caps sizes at 1 TB.
The hex partition type accepts a leading sign (-1 becomes 0xFF) and values over 0xFF.
read_sets stops silently at 32 sets.
A long Summary can clip on 80×25, so the final "Install?" line can fall off.
A truncated driverDetect output tail may parse a partial first line as a bogus driver.
The password hash stays in static memory across "Start over".
Nothing warns on the Summary if no disk controller is ticked (the installed disk then won't boot).
The "target still mounted" error screen was never exercised on a guest; only its st_dev check was.

Media builder (vm/instmedia)

Zero sets gives one "package X is in no set" line per package instead of one "no sets found" message.
parse_set decodes ASCII, so a non-ASCII byte gives a bare UnicodeDecodeError.
read_sets ignores a symlinked set file.
live.compose and build.build each rebuild a tree for read_sets.
The TABLE fixture is simpler than the real template, so set_key is only tested against it.
The live.py docstring says the overlay adds /private/Devices "only"; it's unconditional now.
docs/superpowers/specs/2026-09-22-install-media-design.md row 7 still shows the two-run apk add flow.

Harness (vm/qemu_boot.py)

--keep-hd0 protects by real-path basename and location only; a hard link under another name would pass.
No tests for symlink or relative-path tricks in check_keepable.
--nic pins PCI slot 3, which --esp's unaddressed virtio disk would take first.

Other

No test pins that old label sectors in LBA 1–2047 can't survive an Auto install (judged low risk: label lookup follows the new 0xA7 entry).
The Instance0-i386.table template comment says the booter probes NE2K by PCI IDs; the kernel's PCIKernBus does.
Our drvPCIBus answers only the doubled-paren PCI_Maximums(( / PCI_ConfigReg(( names; DR2's answers PCIMaximums( / PCIConfigReg(. driverDetect works around it with three spellings; the prefix table is likely a reconstruction slip.
NE2K kernel messages draw over sysinstall's curses screen.
Installed filesystems come out big-endian (disk -i on the guest), so vm/rhap_image.py can't read them.
qemu_boot.py --type always presses Return; --keys types without it (added in phase 5a), so this only matters for menu-driving by habit.

Phase 6 (bootable ISO, merged in `c6c392a00`)

Record: docs/build/instmedia-cd.md, design docs/superpowers/specs/2026-10-07-bootable-iso-design.md.

bootefi

The fallback "first labelled disk" can pick the installer CD over the installed disk in the two-disk layout (loader on an ESP-only disk) if the firmware lists the CD first. Prefer a 512-byte labelled disk in the fallback.
disk.c's 18-sector read-ahead isn't clamped to the media end; on the CD the live UFS runs to the last block, so a file in the last ~9 KB would fail to read. Clamp nblocks to Media->LastBlock.
The bounce-buffer guard (UINTN)nblocks * CD_BLOCK can wrap on IA32; use nblocks > sizeof(bounce) / CD_BLOCK. Unreachable today (requests are at most 18 sectors).
efi_cd_label_sector(block_size) ignores its argument.
No tests for nsecs == 0, the 32-bit wrap or reads at the media end; the reversed El Torito disk-select test case was never seen RED on its own.
efi_console.c's gets() stub comment says it is never exercised, but stringTable's Query prompt can reach it.
bootefi shows no missing-boot-driver notice (boot2 does).

boot-2 cleanup

The missing-driver notice clears errors like the removed prompt did, so an unrelated earlier error skips its BOOT_TIMEOUT pause when a driver is also missing. Saving and restoring the value would be tidier.
drivers.c declares extern BOOL errors while bootefi defines int errors (pre-existing; the low byte is cleared).
Stale leftovers: src/system_config-1/i386/Instance0.network still sets "Ask For Drivers", "Prompt For Driver Disk" and DRIVER_ASK_3; boot-2/reconstruction/vbe/divergences.md, saio.def (not built) and a FLOPPY note in boot2's Makefile header; the BootHelp fd() example and unused driver-disk strings.

Builder (vm/instmedia)

compose(preinstalled=True, form="cd") ignores form, and an unknown form string is treated as disk (build refuses the bad combinations).
test_cd_label_fields doesn't assert ntracks/nsectors/ncylinders/d_type or partition a's fsize/bsize/cpg; cd_label has a redundant lbl = …; return lbl.
test_boot_image_ends_on_a_cylinder compares against bui._part_entry itself; literal CHS bytes would be stronger. boot_image's fs_sectors is only an upper bound (the UFS fills the partition) — worth a docstring line.
iso.write_iso re-runs layout() and doesn't assert BOOT.CAT's LBA equals catalog_lba before writing (only check_iso after); total_sectors isn't checked against the layout end.
build.py's boot-image fs_sectors runs outside the NoSpace→BuildError try; release_line decodes software_version as ASCII only.
No package version bumps for kernel-7, boot-2, cdis-3 or diskdev_cmds for these changes, although master bumps them explicitly.

Harness (vm/qemu_boot.py)

The --keep-hd0 same-file check leaves out --cdrom, so an ISO under work/pN-* passed as both IMAGE and --cdrom would be written through hd0.
With --cdrom, hd0 gets no fallback bootindex; worth a help-text line.
Over-long lines in run()'s signature and two test kwargs lines.

Drivers and templates

The CDIS Instance0-i386.table template comment still describes the old Boot Drivers list (and says the booter probes NE2K; the kernel does).
drvAHCI logs "AHCIATAPIController returns nil for +requiredProtocols" about 11 times per boot, onto the installer's screens.
disk.tproj/disk.c keeps its floppy do_boot1 machinery, now a silent no-op unless -B1 is given.

Originally created by @raynorpat on GitHub (Oct 6, 2026). Original GitHub issue: https://github.com/RhapsodiOS/RhapsodiOS/issues/36 Minor findings the phase 5a reviews deferred (installer merged in 197a04b60; design `docs/superpowers/specs/2026-10-05-sysinstall-design.md`, record `docs/build/sysinstall.md`). None blocks use. **Disk layout (`layout.c`)** - Missing edge tests: the 16- and 64-head bands, the `total > 63*255*1024` branch, CHS at cylinder 1023 vs 1024, decoding a garbage signed MBR. - An active byte other than 0x80 is normalised to inactive on re-encode. - `layout_check` doesn't refuse disks under 1 GB itself; only `layout_auto` does. - An entry at LBA 0 or over the MBR sector isn't rejected. - `test_check_rules` asserts only non-NULL, not which rule fired. **Sets and config (`sets.c`, `config.c`)** - A `title` line after the first package line is read as a package named "title". - A package line with embedded whitespace is accepted as one name. - An over-long title or description rejects the whole file (untested). - The password hash isn't checked for `:` or newline (crypt output can't contain them). - Missing tests: a root line without a trailing newline, a CRLF `master.passwd`, `make_salt` determinism, one set naming a package twice. **`driverDetect`** - `Auto Detect IDs` values of 1024 bytes or more are skipped silently. - The table parser stops at the first malformed entry. - A table with no `Location` key gets a second `Location` appended (untested). - Exits 0 with an empty list when `/usr/Devices` is missing; sysinstall can't tell that from "no cards". - `src/driverkit-3/PB.project` doesn't list `driverDetect`. - An unchecked `malloc` for table names; `t2` leaks if `spew` fails in `writeRoot`. - The Makefile omits `tests/` from `SOURCEFILES` and hardcodes i386 paths; the ppc slice was never compiled. - The full PCI scan costs up to 256 buses × 32 devices of calls; match order follows `readdir` order. - No host tests for `replaceDisk`, `rootDisk`, or the Boot/Active composition in `-w`. **Write phase and screens (`steps.c`, `main.c`, `ui.c`, `disks.c`)** - The `pclose` status of the ESP stream is ignored, so a gzip failure reads as a size error. - `master.passwd`'s mode relies on the apk shipping 0600; an explicit chmod would be safer. - The step 6 parent directories must exist from the apks; otherwise the error is generic. - Missing tests: a missing or short `boot0`, the disk-name bound. - `int blocks` in the `DKIOCNUMBLKS` call caps sizes at 1 TB. - The hex partition type accepts a leading sign (`-1` becomes 0xFF) and values over 0xFF. - `read_sets` stops silently at 32 sets. - A long Summary can clip on 80×25, so the final "Install?" line can fall off. - A truncated `driverDetect` output tail may parse a partial first line as a bogus driver. - The password hash stays in static memory across "Start over". - Nothing warns on the Summary if no disk controller is ticked (the installed disk then won't boot). - The "target still mounted" error screen was never exercised on a guest; only its st_dev check was. **Media builder (`vm/instmedia`)** - Zero sets gives one "package X is in no set" line per package instead of one "no sets found" message. - `parse_set` decodes ASCII, so a non-ASCII byte gives a bare `UnicodeDecodeError`. - `read_sets` ignores a symlinked set file. - `live.compose` and `build.build` each rebuild a tree for `read_sets`. - The `TABLE` fixture is simpler than the real template, so `set_key` is only tested against it. - The `live.py` docstring says the overlay adds `/private/Devices` "only"; it's unconditional now. - `docs/superpowers/specs/2026-09-22-install-media-design.md` row 7 still shows the two-run `apk add` flow. **Harness (`vm/qemu_boot.py`)** - `--keep-hd0` protects by real-path basename and location only; a hard link under another name would pass. - No tests for symlink or relative-path tricks in `check_keepable`. - `--nic` pins PCI slot 3, which `--esp`'s unaddressed virtio disk would take first. **Other** - No test pins that old label sectors in LBA 1–2047 can't survive an Auto install (judged low risk: label lookup follows the new `0xA7` entry). - The `Instance0-i386.table` template comment says the booter probes NE2K by PCI IDs; the kernel's `PCIKernBus` does. - Our drvPCIBus answers only the doubled-paren `PCI_Maximums((` / `PCI_ConfigReg((` names; DR2's answers `PCIMaximums(` / `PCIConfigReg(`. `driverDetect` works around it with three spellings; the prefix table is likely a reconstruction slip. - NE2K kernel messages draw over sysinstall's curses screen. - Installed filesystems come out big-endian (`disk -i` on the guest), so `vm/rhap_image.py` can't read them. - `qemu_boot.py --type` always presses Return; `--keys` types without it (added in phase 5a), so this only matters for menu-driving by habit. ## Phase 6 (bootable ISO, merged in c6c392a00) Record: `docs/build/instmedia-cd.md`, design `docs/superpowers/specs/2026-10-07-bootable-iso-design.md`. **`bootefi`** - The fallback "first labelled disk" can pick the installer CD over the installed disk in the two-disk layout (loader on an ESP-only disk) if the firmware lists the CD first. Prefer a 512-byte labelled disk in the fallback. - `disk.c`'s 18-sector read-ahead isn't clamped to the media end; on the CD the live UFS runs to the last block, so a file in the last ~9 KB would fail to read. Clamp `nblocks` to `Media->LastBlock`. - The bounce-buffer guard `(UINTN)nblocks * CD_BLOCK` can wrap on IA32; use `nblocks > sizeof(bounce) / CD_BLOCK`. Unreachable today (requests are at most 18 sectors). - `efi_cd_label_sector(block_size)` ignores its argument. - No tests for `nsecs == 0`, the 32-bit wrap or reads at the media end; the reversed El Torito disk-select test case was never seen RED on its own. - `efi_console.c`'s `gets()` stub comment says it is never exercised, but stringTable's Query prompt can reach it. - `bootefi` shows no missing-boot-driver notice (boot2 does). **boot-2 cleanup** - The missing-driver notice clears `errors` like the removed prompt did, so an unrelated earlier error skips its BOOT_TIMEOUT pause when a driver is also missing. Saving and restoring the value would be tidier. - `drivers.c` declares `extern BOOL errors` while bootefi defines `int errors` (pre-existing; the low byte is cleared). - Stale leftovers: `src/system_config-1/i386/Instance0.network` still sets "Ask For Drivers", "Prompt For Driver Disk" and `DRIVER_ASK_3`; `boot-2/reconstruction/vbe/divergences.md`, `saio.def` (not built) and a FLOPPY note in boot2's Makefile header; the BootHelp `fd()` example and unused driver-disk strings. **Builder (`vm/instmedia`)** - `compose(preinstalled=True, form="cd")` ignores `form`, and an unknown form string is treated as disk (`build` refuses the bad combinations). - `test_cd_label_fields` doesn't assert ntracks/nsectors/ncylinders/d_type or partition a's fsize/bsize/cpg; `cd_label` has a redundant `lbl = …; return lbl`. - `test_boot_image_ends_on_a_cylinder` compares against `bui._part_entry` itself; literal CHS bytes would be stronger. `boot_image`'s `fs_sectors` is only an upper bound (the UFS fills the partition) — worth a docstring line. - `iso.write_iso` re-runs `layout()` and doesn't assert `BOOT.CAT`'s LBA equals `catalog_lba` before writing (only `check_iso` after); `total_sectors` isn't checked against the layout end. - `build.py`'s boot-image `fs_sectors` runs outside the NoSpace→BuildError try; `release_line` decodes `software_version` as ASCII only. - No package version bumps for kernel-7, boot-2, cdis-3 or diskdev_cmds for these changes, although master bumps them explicitly. **Harness (`vm/qemu_boot.py`)** - The `--keep-hd0` same-file check leaves out `--cdrom`, so an ISO under `work/pN-*` passed as both IMAGE and `--cdrom` would be written through hd0. - With `--cdrom`, hd0 gets no fallback `bootindex`; worth a help-text line. - Over-long lines in `run()`'s signature and two test kwargs lines. **Drivers and templates** - The CDIS `Instance0-i386.table` template comment still describes the old Boot Drivers list (and says the booter probes NE2K; the kernel does). - drvAHCI logs "AHCIATAPIController returns nil for +requiredProtocols" about 11 times per boot, onto the installer's screens. - `disk.tproj/disk.c` keeps its floppy `do_boot1` machinery, now a silent no-op unless `-B1` is given.

gitea-mirror added the

bug

enhancement

labels

2026-11-03 22:02:34 -07:00

gitea-mirror commented

2026-11-03 22:02:35 -07:00

Author

Owner

@raynorpat commented on GitHub (Oct 7, 2026):

disk.tproj/disk.c: removed the floppy do_boot1 no-op machinery in commit 240802e8d

@raynorpat commented on GitHub (Oct 7, 2026): disk.tproj/disk.c: removed the floppy do_boot1 no-op machinery in commit 240802e8d

gitea-mirror referenced this issue

2026-11-03 22:04:29 -07:00

[PR #29] [MERGED] drivers: drop the append-duplicated Server Name line from driver config tables #84