Fix the package fs seam: UTF-8 paths, GetUserFileName pickers, exclusive-create landing, rollback arm/disarm
Both pickers now ride GetUserFileName (mode 0/1); the "no save picker" premise was false. Landing uses O_EXCL so the create is the existence check, not a TOCTOU pair.
This commit is contained in:
+70
-40
@@ -3,59 +3,89 @@
|
||||
## Scope
|
||||
|
||||
The filesystem and dialog acts behind bank-package export/import: streaming package
|
||||
file I/O (`package_io`), the landed-file journal and its rollback delete
|
||||
(`package_rollback`), and the platform file pickers (`package_pickers`). This seam
|
||||
is bytes-only — the package format (magic, manifest, entry layout) is
|
||||
`core/package`'s business, and the export/import verbs that orchestrate both do not
|
||||
live here yet. No REAPER project state is touched in this directory: no ext-state
|
||||
read or write, no undo block, no generation bump — those belong to the verbs.
|
||||
file I/O plus the file-status and exclusive-create acts (`package_io`), the
|
||||
UTF-8 path conversion every one of them goes through (`package_path`), the landed-file
|
||||
journal and its rollback delete (`package_rollback`), and the two file pickers
|
||||
(`package_pickers`). This seam is bytes-only — the package format (magic, manifest,
|
||||
entry layout) is `core/package`'s business, and the export/import verbs that
|
||||
orchestrate both do not live here yet. No REAPER project state is touched in this
|
||||
directory: no ext-state read or write, no undo block, no generation bump — those
|
||||
belong to the verbs.
|
||||
|
||||
## Invariants
|
||||
|
||||
- **Atomic write.** A package accumulates in a `.rsbanktmp` sibling in the
|
||||
destination directory and reaches the destination only through `commit()`'s
|
||||
rename (the mono-collapse temp+rename precedent). A failed, aborted, or abandoned
|
||||
write leaves the destination absent or holding its prior contents — never a
|
||||
partial `.rsbank`.
|
||||
- **Paths cross this seam as UTF-8 narrow strings and are converted through
|
||||
`utf8Path()` before ANY filesystem call.** This is not decoration: on Windows
|
||||
`std::filesystem` decodes a narrow path through the runtime ANSI code page (measured
|
||||
`GetACP() == 1252`), so a bare `fs::path(std::string)` turns `café.rsbank` into
|
||||
`café.rsbank` or fails to open it. Every path a verb hands in or gets back —
|
||||
including `listFolderFileNames`' results, which use `u8string()` and never
|
||||
`string()` — is UTF-8. `core/util/file_bytes` has the un-converted shape, which is
|
||||
why `readFilePayload` reads through this module's own `PackageFileReader` instead.
|
||||
- **Atomic package write, to the limit of a rename.** A package accumulates in a
|
||||
`.rsbanktmp` sibling in the destination directory and reaches the destination only
|
||||
through `commit()`'s rename (the mono-collapse temp+rename precedent). A failed,
|
||||
aborted, or abandoned write leaves the destination absent or holding its prior
|
||||
contents. This is process-crash atomic, NOT power-loss atomic: `commit()` flushes
|
||||
and closes but does not `fsync`/`FlushFileBuffers`, so a power cut can still leave a
|
||||
renamed-but-unflushed file. Deliberate — an fsync over a whole sample bank is a real
|
||||
stall, and the failure this design targets is a refused or interrupted export.
|
||||
- **Streaming, both ways — at most ONE entry's payload in memory.** Writes append
|
||||
one payload at a time; reads seek and materialize one range at a time. The claim
|
||||
is structural, not aspirational: every payload crosses this seam as a move-only
|
||||
`PayloadBuffer`, and `PayloadBuffer::alive()` is the seam counter the tests
|
||||
assert against. There is no read-whole-package or write-whole-package entry
|
||||
point; do not add one.
|
||||
- **The rollback delete is prune's ONE carve-out, cited not restated.** The
|
||||
citation and the discriminator live at `package_rollback.cpp`'s header. The
|
||||
journal makes the discriminator structural: only paths its own `writeLandedFile`
|
||||
successfully created are recorded, and `rollback()` consumes only the record — a
|
||||
path this import did not write cannot be handed to it.
|
||||
- **No overwrite of a bank-folder file, ever.** `writeLandedFile` refuses an
|
||||
existing destination outright; collision handling (auto-rename) is the import
|
||||
plan's job upstream. The package writer itself DOES replace an existing
|
||||
destination — the export save dialog's own overwrite confirm is the consent —
|
||||
and that asymmetry is deliberate.
|
||||
- **The two pickers are asymmetric, and the asymmetry is real.** Import rides
|
||||
REAPER's own `GetUserFileNameForRead` (both platforms); export goes native —
|
||||
Win32 `GetSaveFileNameW` / SWELL `BrowseForSaveFile` — because the always-present
|
||||
REAPER surface offers no save picker. Do not symmetrize; the newer
|
||||
`GetUserFileName(mode=0)` alternative and why it is not used are recorded in
|
||||
`package_pickers.cpp`'s header.
|
||||
- **An empty `PayloadBuffer` is a failure signal, never an entry.** It is the seam's
|
||||
one "nothing to work with" branch, so both `PackageFileWriter::appendPayload` and
|
||||
`writeFileExclusive` refuse it — appending it would let a verb commit framing that
|
||||
claims bytes nobody wrote. `appendRaw(ptr, 0)` stays tolerated: framing has
|
||||
legitimate zero-length edges.
|
||||
- **No overwrite of a bank-folder file, ever — and the create is the check.**
|
||||
`writeLandedFile` lands through `writeFileExclusive` (`O_EXCL` / `_O_EXCL`), so the
|
||||
refusal of an occupied path is one atomic act rather than an `exists()` a concurrent
|
||||
writer could win the race against. Collision handling (auto-rename) remains the
|
||||
import plan's job upstream. The package writer itself DOES replace an existing
|
||||
destination — the export save dialog's own overwrite confirm is the consent — and
|
||||
that asymmetry is deliberate.
|
||||
- **The rollback delete is prune's ONE carve-out, and only HALF of it is structural.**
|
||||
The citation and the full discriminator live at `package_rollback.cpp`'s header.
|
||||
"Did this call create it" is structural: only exclusively-created paths are
|
||||
recorded, resolved absolute at record time so a later CWD change cannot re-aim the
|
||||
delete. "Did anything ever reference it" is a **contract the import verb must
|
||||
honour**: it MUST call `markIndexCommitted()` at the moment it commits the index,
|
||||
after which `rollback()` refuses and `writeLandedFile` refuses.
|
||||
- **Both pickers ride `GetUserFileName`** — mode 1 for import, mode 0 for export.
|
||||
There is no platform split and no fallback: `main.cpp` defines `REAPERAPI_IMPLEMENT`
|
||||
without `REAPERAPI_MINIMAL` and aborts the extension load if any single name fails
|
||||
to resolve, so a build that can load us cannot lack it.
|
||||
|
||||
## Modules
|
||||
|
||||
- `package_io` — the streaming filesystem seam: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), and `listFolderFileNames` (bare names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW.
|
||||
- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (temp+rename land, recorded on success only, refuses an existing destination and an empty payload) and `rollback` (deletes exactly the recorded set, hard unlink — nothing ever referenced these bytes — tolerating a vanished file). REAPER-free; tested without a DAW.
|
||||
- `package_pickers` — the two pickers in one platform TU (`#ifdef _WIN32` / `#else swell/swell.h`, the `draw_kit`/`prune_fs` split): `pickPackageForImport` (REAPER read picker) and `pickPackageSavePath` (native save dialog, UTF-8 in/out on Windows). Compile-only until the verbs land; nothing here can be exercised in a unit test.
|
||||
- `package_path` — header-only; the ONE UTF-8-narrow → `fs::path` conversion, so the encoding contract has a single enforcement point.
|
||||
- `package_io` — every filesystem act the verbs need: `PayloadBuffer` (move-only payload + the `alive()` seam counter), `PackageFileWriter` (append-only temp+atomic-rename writer), `PackageFileReader` (seek-and-read one range per call, range-checked against the real file size), `readFilePayload` (one source file as one entry's payload), `fileStatus` (Present/Absent/Unreadable — export's refusal message must distinguish the last two, and an empty payload cannot), `writeFileExclusive` (exclusive create + write, self-cleaning on a partial write), and `listFolderFileNames` (bare UTF-8 names, sorted, non-recursive, non-throwing). REAPER-free; tested without a DAW.
|
||||
- `package_rollback` — `LandedFileJournal`: `writeLandedFile` (exclusive-create land, path resolved absolute, recorded on success only), `markIndexCommitted` (disarms the journal), and `rollback` (deletes exactly the recorded set, hard unlink, tolerating a vanished file; refuses once disarmed). REAPER-free; tested without a DAW.
|
||||
- `package_pickers` — `pickPackageForImport` and `pickPackageSavePath`, both `GetUserFileName`. Compile-only until the verbs land; nothing here can be exercised in a unit test.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- A crash mid-write strands the `.rsbanktmp` sibling. It is not a `.rsbank` (no
|
||||
picker filter matches it), and a later export to the same destination truncates
|
||||
it — but one stranded in the BANK folder by a mid-import crash is a foreign file
|
||||
to prune (not owned, so never an orphan) until removed by hand. `[verify — DAW]`
|
||||
whether the import verb should pre-clean stale `.rsbanktmp` names when it lands.
|
||||
- The picker `defext`/filter strings are spelled to the Win32 `lpstrDefExt`
|
||||
convention (no dot) but are `[verify — DAW]` on all three platforms — neither
|
||||
picker is exercised outside a live REAPER session.
|
||||
- A crash mid-export strands the `.rsbanktmp` sibling. It is not a `.rsbank` (no
|
||||
picker filter matches it), and a later export to the same destination truncates it.
|
||||
A crash mid-import strands a partial bank file under its real name instead — the
|
||||
land is a direct exclusive create, not temp+rename. Either way the debris was never
|
||||
recorded in the tracking ledger, so prune sees a foreign file (not owned, never an
|
||||
orphan) and will not touch it; removal is by hand. `[verify — DAW]` whether the
|
||||
import verb should pre-clean stale debris when it lands.
|
||||
- The picker filter and mode arguments are spelled to `GetUserFileName`'s documented
|
||||
pair format but are `[verify — DAW]` on all three platforms — neither picker is
|
||||
exercised outside a live REAPER session. `GetUserFileName` also takes no owner
|
||||
window, so dialog parenting is REAPER's to do; the superseded Win32 path passed
|
||||
`GetMainHwnd()` explicitly.
|
||||
- `pickPackageSavePath`'s `suggestedPath` doubles as the dialog's starting directory
|
||||
when it is a full path. The verbs should seed it from the project directory —
|
||||
passing a bare name leaves the dialog on REAPER's process working directory, which
|
||||
is its install or resource path.
|
||||
- `readRange(_, 0)` returns an empty buffer — indistinguishable from failure, by
|
||||
design (the one "nothing to work with" branch). A genuinely zero-length entry
|
||||
cannot round-trip through this seam; the format layer must not emit one.
|
||||
design (the one "nothing to work with" branch). **Cross-track contract, not a local
|
||||
rule:** a genuinely zero-length entry cannot round-trip through this seam, so
|
||||
`core/package`'s format layer must not emit one.
|
||||
|
||||
Reference in New Issue
Block a user