Repository navigation
feat(libSceSaveData): implement sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup - #12748
Conversation
…taRestoreBackupData and sceSaveDataUmountWithBackup
Progress report (b28bb77 → 02af078)📈 System libraries: 26.76% (+0.01%, +3 implemented functions) ✅ 3 implemented
Changed libraries (implemented exports)
|
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
|
@EagleBearerYS The three functions follow shadPS4's binding closely and the struct layouts match it field for field. Because they write the player's save on disk, I went through the failure paths. Two independent reviews looked at this; the points below are from both. Five things I would like changed, and some tests to match:
Tests I could not find in Checked and fine: directory names are validated by one shared helper (no The existing merge order after #10674 still applies. |
…e read it sceSaveDataCheckBackupData and sceSaveDataRestoreBackupData now read a backup through one check that runs before anything in the live save changes. A backup directory without a save inside, a missing .param, a .param that is not exactly one SaveDataParam, or a blocks entry that is not a file returns SAVE_DATA_ERROR_INTERNAL, whether or not the caller asked for param. Before, the check threw on a wrong-sized .param, and the restore removed the live save's files before finding the .param missing.
…back a failed swap sceSaveDataRestoreBackupData used to remove the live save's files and copy the backup over them, so a copy that failed half way left neither. It now copies the backup, its .param and its blocks file to _sd_restore_tmp/<dir>, checks that the copy has every entry at its size, and flushes it. Then it moves the live save, .param and blocks file to _sd_restore_old/<dir>, moves the staged copy into place and removes the old one. A failed move puts back everything already moved, so the live save stays as it was. The staging roots sit next to _sd, not in it, where sceSaveDataDirNameSearch would list them. SaveDataSetFileStepHook_nid_no_patch sets a function that is called before each copy and rename of the backup and restore paths with a step name and the path written; when it returns false that step fails. It does nothing unless a test sets it.
…ls, and flush before removing it backupSave moved the previous backup to _sd_backup_old and then renamed the new copy into place; when that rename failed, the save had no backup until the next one. A failed rename now moves the previous backup back into _sd_backup and removes the staged copy. Every staged file is flushed before the swap (FlushFileBuffers on Windows, fsync on POSIX, as libc does), and on POSIX the directories on both sides of each rename are flushed too, all before the previous backup is removed.
Before any copy or delete, the backup and the restore check the roots they use (_sd, _sd_backup and their staging and old directories) and every entry under them for the save: each must be a plain directory or file, not a symbolic link, or on Windows a reparse point such as a junction, and must stay under its root after weakly_canonical. Anything else makes the restore return SAVE_DATA_ERROR_INTERNAL, and the backup report it in its event, with nothing changed. sceSaveDataCheckBackupData checks the backup the same way. The copies walk the tree themselves and refuse a link instead of following it.
Checks behaviour 5fed277 already has: a second sceSaveDataUmountWithBackup replaces the first backup; a backup copy that fails still unmounts and returns 0, with SAVE_DATA_ERROR_INTERNAL in its event and the previous backup kept; another title's titleId is SAVE_DATA_ERROR_NOT_FOUND for the check and the restore; ".." and "a/b" are SAVE_DATA_ERROR_PARAMETER for the check, the restore and the mount a backup needs, and as mount points sceSaveDataUmountWithBackup reports them SAVE_DATA_ERROR_NOT_MOUNTED, as it does for any unknown mount point. The sizes and field offsets of SaveDataCheckBackupData and SaveDataRestoreBackupData are checked with static_assert.
…save Three entries in the libSceSaveData.native debt file: a failed restore leaves the live save as it was, and where a crash during a restore or a backup leaves the save and its backup, since neither is recovered at the next start; and that on Windows directory entries cannot be flushed, so crash safety there rests on NTFS journaling and is not shown.
|
@oneandonlydean thanks for going through the failure paths. All five points are addressed in follow-up commits on this PR, now at b17c475, with the tests you listed.
Tests. eff8683 adds the coverage cases; the others come with their fixes.
On Linux, each fix commit's new cases fail on its parent and pass with it. eff8683's cases already pass on this PR's original head, so that commit adds coverage only. Runs on GitHub's runners at b17c475:
Not covered: a real crash or power loss during a backup or restore (the debt entries say what it would leave). Nothing here is measured on a console, and the existing PS4-source note stays. Could you take another look? |
|
@EagleBearerYS A console run of the three exports: PS4 Pro, firmware 10.71, the system So the console agrees with this PR on every row it covers: One thing the PR does not model: AI-assisted: yes (Claude Code) |
ntamas94 ran sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData and sceSaveDataUmountWithBackup on a PS4 Pro with firmware 10.71's system libSceSaveData.sprx. The console agrees with this library on every case it covers. Two of its rows go in the debt file: another title's titleId returns NOT_FOUND, and sceSaveDataDelete drops the save's backup as this library does, which the entry called unknown. The run also shows the check returning PARAMETER for a param buffer filled with 0xAA, and 0 for a zeroed or null one. Which bytes the console checks was not measured, so the check is not changed; a new entry states the measured cases and that the byte set is unknown.
|
@ntamas94 thank you, this is the first console data for these three calls. Every row you covered matches what the PR does. Two of them settle points the debt file had left open: another title's The non-zeroed |
# Conflicts: # core/libs/tests/GuestSaveDataMountStatus.cpp # docs/dev/debt/libSceSaveData.native.md
# Conflicts: # core/libs/tests/GuestSaveDataMountStatus.cpp # docs/dev/debt/libSceSaveData.native.md
JJenkx's boykopovar#13324 gives isMountedDirectory an std::error_code parameter. The merge passes one in checkBackupData and restoreBackupData and returns SAVE_DATA_ERROR_INTERNAL on a host error, as main's callers do. GuestSaveDataMountStatus.cpp: CheckBackups writes param.json where ResolvePath_nid_no_patch puts /app0/sce_sys/param.json, not under the working directory. boykopovar#13324's CheckHostMountFailures now resolves the first guest path from its mount-host-failures subdirectory, so /app0 maps there, and checkBackupData didn't find the title's param.json.
# Conflicts: # core/libs/prx/libSceSaveData.native/Export.cpp
# Conflicts: # docs/dev/debt/libSceSaveData.native.md The pending declarations line keeps main's sceSaveDataGetProgress split and drops the three names this branch implements (sceSaveDataCheckBackupData, sceSaveDataRestoreBackupData, sceSaveDataUmountWithBackup).
# Conflicts: # docs/dev/debt/libSceSaveData.native.md
ea57a10
What
The three were parameterless declarations that threw through
NotImplemented_nid_no_patch. They now take shadPS4's signatures and do what its PS4 binding does, on this library's save store.sceSaveDataCheckBackupData, with its block layout;sceSaveDataRestoreBackupDataand itsRestore, with its block layout;sceSaveDataUmountWithBackup, which isUmountfollowed by a backup and aUMOUNT_BACKUPevent. fpPS4's version unmounts and posts the event.SAVE_DATA_ERROR_NOT_INITIALIZEDbefore initialization;PARAMETERfor a null block or directory name;BUSYwhile the save is mounted;NOT_FOUNDwhen there is no backup;paramfilled from the backup.INTERNALwhen the save directory is missing, the backup's metadata is incomplete or the wrong size, or a file call fails._sd_restore_tmp/<dir name>, checked complete and flushed. The live save then moves to_sd_restore_old/<dir name>and the copy into place, and a failed move puts back what had moved, so a failed restore leaves the save as it was.NOT_INITIALIZEDbefore initialization, andPARAMETERfor a null mount point;sceSaveDataGetEventResult;INTERNAL, as shadPS4's backup thread does, and the call still returns 0.libSceSaveData.native.md's debt entries:NOT_MOUNTED, assceSaveDataUmount2does here, where shadPS4 returnsNOT_FOUND;BACKUP_BUSYnever occurs and the event is queued at once, as in fpPS4;titleIdreturnsNOT_FOUND, because only the running title's saves are kept;icon. shadPS4 does the same when there's noicon0.png._sd_backup/<dir name>, outside the save, so the title's mount and its block count don't see them._sd_backup_tmp, flushed, and swapped in through_sd_backup_old, like shadPS4'ssce_backup_tmpandsce_backup_old. A failed swap puts the previous backup back.INTERNALwith nothing changed.sceSaveDataDeleteremoves a save's backup along with the save. In shadPS4 the backup lives inside the save directory that Delete removes.mount3,deleteSaveandconverteach had their own copy of the directory-name check. They and the two new functions now call one helper, with the same rules.MountSlotnow keeps the mounting user, for the event.PARAMETERfor aparambuffer that isn't zeroed, is a debt entry, since which bytes the console checks isn't known.sceSaveDataBackupdoesn't copy yet, so the check and the restore see only backups thatsceSaveDataUmountWithBackupmade (also in the debt entry). Making it use the same copy is a follow-up.Tested
guest_savedata_mount_status, 37 new checks:sceSaveDataInitialize3, all three calls returnNOT_INITIALIZED;PARAMETERfor a null block or directory name, and for a null mount point;NOT_FOUNDfor a save with no backup, andBUSYfor a mounted one, from both the check and the restore;sceSaveDataUmountWithBackupreturns 0 and unmounts.sceSaveDataGetEventResultthen gives type 1, error 0, user 7, titlePPSA00000and directorykept;paramwith the backed-up title, also when it is given the running title's ID;data.binis changed,added.binis added and the title is changed, the restore puts backdata.bin, removesadded.binand puts back the title;NOT_FOUND.main's library code with only the three signatures changed so it builds: the test aborts withsceSaveDataCheckBackupData not implemented.sceSaveDataDelete: only "deleting a save deletes its backup" fails.guest_savedata_*,savedata_*) ran 10 times: 130 passed, 0 skipped, 0 failed.main's code doesn't build the new cases, because its stubs take no parameters.check_test_registration.pypasses, and no comment lines are added.check_conventions.py: one warning, the debt-length check on the pending list inlibSceSaveData.native.md. That list is a single line and was already over the limit onmain. This change removes three names from it, so it gets shorter, from 6,140 to 5,870 characters.-fsyntax-only:Export.cppand the test show no new diagnostics.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.ctest -V, 2 runs) shows no case skipped on either runner, so the junction and reparse-point cases ran on Windows and the symlink cases on Linux.libSceSaveData.sprx(SDK 0x05050001), the shadPS4 structures and a zeroed 1,328-byteparam. It agrees with this PR on every row it covers:NOT_FOUNDwith no save or backup, for another title's id or another directory, and aftersceSaveDataDelete, so the delete takes the backup with it;BUSYwhile the save is mounted, beforeNOT_FOUND;NOT_MOUNTEDfromsceSaveDataUmountWithBackupfor a mount point that is no longer mounted;PARAMETERfor aparamfilled with 0xAA, also with a backup present, and 0 for a zeroed or null one. Which bytes it checks wasn't measured, so bb346f7 records it as a debt entry, with the delete and other-title rows, and the code is unchanged. The events weren't measured, and this is a PS4, not a PS5.main91a6995 (run). Four of themainchanges it brings touch this PR's files: slopgobblercobbler's fix(libSceSaveData.native): validate directory name in sceSaveDataBackup #12644, ionut27001's fix(libSceSaveData.native): answer sceSaveDataCancel before initialization as measured in a PS5 payload #11720, al3w0f205's test(libSceSaveData.native): cover unmounted mount point in icon operations #12220 and xXPBSXx's docs(debt): count how local titles pair sceSaveDataInitialize3 and Terminate #12600.sceSaveDataBackup's directory name.GuestSaveDataMountStatus.cpp, fix(libSceSaveData.native): validate directory name in sceSaveDataBackup #12644 adds its backup checks at the end ofCheckUnmountedCalls, where this PR adds its threeNOT_INITIALIZEDchecks. The merge keeps both: fix(libSceSaveData.native): validate directory name in sceSaveDataBackup #12644's checks first, then this PR's.docs/dev/debt/libSceSaveData.native.md, fix(libSceSaveData.native): validate directory name in sceSaveDataBackup #12644 adds an entry forsceSaveDataBackup's unterminated directory name where this PR adds its entries for the three backup calls. The merge keeps fix(libSceSaveData.native): validate directory name in sceSaveDataBackup #12644's entry, then this PR's.check_conventions.py'sdebt-lengthwarning on line 19 ismain's pending-declarations entry: 6,140 characters onmain, 5,870 here after this PR removes the three names it implements.^guest_savedata, 7 tests.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main8c8b998 (run), which brings SnekPolylepis's feat(libSceSaveData): add provisional abort and clear progress stubs #12699 (sceSaveDataAbort,sceSaveDataClearProgressand the mounted-save count).GuestSaveDataMountStatus.cppand one indocs/dev/debt/libSceSaveData.native.md, all with feat(libSceSaveData): add provisional abort and clear progress stubs #12699. The merge keeps both sides:CheckMountCapacityAndLifecycle;main(), this PR's backup checks run before feat(libSceSaveData): add provisional abort and clear progress stubs #12699's closingsceSaveDataTerminatechecks;sceSaveDataAbort,sceSaveDataClearProgressandsceSaveDataGetMountedSaveDataCount. 60 names remain.SaveDataErrorNotInitializedconstant this PR's test added, so that line is nowmain's. This PR's other added and removed lines are unchanged.^guest_savedata, 7 tests.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main7fcab08, which brings JJenkx's fix(savedata): keep failed host mounts unpublished #13324 (run).isMountedDirectoryanstd::error_codeparameter, and this PR's two new calls, incheckBackupDataandrestoreBackupData, no longer compiled onmain. The merge passes one, asmain's callers do, so Check and Restore now returnSAVE_DATA_ERROR_INTERNALon a host filesystem error instead of throwing, consistent with fix(savedata): keep failed host mounts unpublished #13324.CheckBackupsnow writes param.json at the resolved/app0path, since fix(savedata): keep failed host mounts unpublished #13324's test fixes the guest root in a subdirectory first.^guest_savedata_and^savedata_,guest_savedata_mount_statusamong them.APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.maind8b60b4, which brings DealPotato's feat(savedata): add sceSaveDataMount2 #8285 (sceSaveDataMount2) and the host free-space checks inmount3.libSceSaveData.native/Export.cpp's include block, where feat(savedata): add sceSaveDataMount2 #8285 adds<string_view>beside this PR's includes. Both are kept.mount3, which both sides change, merged on its own and composes:mainreads the free-space mode first and refuses a new save the host volume can't hold, and this PR'sparse_dir_nameand the slot'suser_idstay. feat(savedata): add sceSaveDataMount2 #8285'smount2passesuser_idthrough to it.guest_savedata_*tests,main's and this PR's. Linux: 8 run, 8 pass, 0 skip, 0 fail. Windows: 8 run, 8 pass, 0 skip, 0 fail. The libc and libkernel suites run 208 and 309 tests on Linux and 235 and 330 on Windows, with 0 failed. The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.main24238c4. The one conflict was the pending-declarations list indocs/dev/debt/libSceSaveData.native.md: MarcZX10's feat(libSceSaveData): declare sceSaveDataGetProgress with its known signature #10674 movessceSaveDataGetProgressout of it into its own entry, and the merge keeps that with this PR's removal of its three calls. This PR's diff overmainis unchanged. On GitHub's runners (run, on 5a7a8f3, a merge ofmain3c3b3cd with the same resolution;main's later commits change only a cmake line and two debt files outside this PR), the^guest_savetests withguest_savedata_mount_statusran 10 times with no failure. Linux: 8 run, 8 pass, 0 skip, 0 fail. Windows: 8 run, 8 pass, 0 skip, 0 fail. Suites: libc and libkernel pass on Linux (214 and 316) and on Windows (241 and 337). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.mainb28bb77. The one conflicting file wasdocs/dev/debt/libSceSaveData.native.md. The merge keepsmain'ssceSaveDataGetProgressentry, which xecedemon's docs(libSceSaveData): survey the pending exports across citable reimplementations #15077 extends, and the unterminated-mount-point sentence that slopgobblercobbler's fix(libSceSaveData.native): validate mount point termination in mount operations #12853 adds to the null-pointer entry, with this PR's removal of its three calls and its new entries. fix(libSceSaveData.native): validate mount point termination in mount operations #12853 and slopgobblercobbler's test(libSceSaveData.native): cover offset boundaries, null buffers, and uninitialized access in save data memory #12846 changelibSceSaveData.native/Export.cpptoo, and those merged without conflicts.mainare identical to those in 3547c39, our merge ofmainf06d7fd, which ran ×10 on GitHub's runners (run): the^guest_savetests withguest_savedata_mount_status. Linux: 8 run, 8 pass, 0 skip, 0 fail. Windows: 8 run, 8 pass, 0 skip, 0 fail. Suites: libc and libkernel pass on Linux (217 and 318) and on Windows (244 and 339). The suites ran withAPS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.guest_savedata_*tests 10 times with no failure on both runners (run).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)mainkeeps both sets of removals and its new entry. Any order with fogydes's fix(libSceSaveData.native): validate guest pointers across convert, backup, mount, and icon operations #15042, for merge order only: inconvertinlibSceSaveData.native/Export.cpp, fix(libSceSaveData.native): validate guest pointers across convert, backup, mount, and icon operations #15042 checks its own copy of the directory name and this PR readsparam->dir_namethroughparse_dir_name. Whichever lands second runsparse_dir_nameon fix(libSceSaveData.native): validate guest pointers across convert, backup, mount, and icon operations #15042's copy.