Handle file install failures more gracefully #1534
Conversation
|
Oh and FWIW, this certainly deserves further test-cases but don't let that stop reviewing... |
|
Do you have any suggested test patterns beyond just "replace regular RPM and do standard DNF stuff"? |
|
Of particular interest are various failure scenarios, whether due to malformed payload (which you have to force with --noverify) or fs-level issues such as immutable files preventing operation, running out of disk-space etc. Two things to watch out for: that no temporary files are left behind, and that no permanent files get accidentally removed. |
|
Minor nit pick: The name and usage of the new |
|
Do we really want to call |
|
Yup, the file plugin calls are buggy here. |
|
File-post slot calls fixed to be called only on files we actually processed, which is in line with the former "in pairs" policy. Also added names to the "stages", these don't seem particularly meaningful at this stage (pun intended or not) of development but certainly no worse than the abstract numbers either. I'm really tempted to move the pre- and post-slots around fsmCommit(), but this would mean those would be called after creation for directories 😒 |
|
Added some tests to demonstrate our new clean-up capabilities. |
Handle rpmfiNext() in the while-condition directly to make it more like similar other constructs elsewhere, adjust for the end of iteration code after the loop. Also take the file index from rpmfiNext() so we don't need multiple calls to rpmfiFX() later.
Collect the common state info into a struct shared by both file install and remove, update code accordingly. The change looks much more drastic than it is - it's just adding fp-> prefix to a lot of places. While we're at it, remember the state data throughout the operation. No functional changes here, just paving way for the next steps which will look clearer with these pre-requisites in place.
Move setmeta into the struct too (we'll want this later anyhow), and now we only need to pass the struct to fsmMkfile(). One less argument to pass around, it has way too many still. No functional changes.
We only use a temporary suffix for regular files that we are actually creating, skipped and touched files should not have it. Add XFA_CREATING() macro to accomppany XFA_SKIPPING() to easily check whether the file is being created or something else. No functional changes but makes the logic clearer.
We only use a temporary suffix for regular files that we are actually creating, skipped and touched files should not have it. Add XFA_CREATING() macro to accomppany XFA_SKIPPING() to easily check whether the file is being created or something else. No functional changes but makes the logic clearer.
No functional changes, just makes it a little cleaner as firstlink now points to the actual file data instead of a index number somewhere.
Run the file installation in multiple stages: 1) gather intel 2) unpack the archive to temporary files 3) set file metadatas 4) commit files to final location 5) mop up leftovers on failure This means we no longer leave behind a trail of untracked, potentially harmful junk on installation failure. If commit step fails the package can still be left in an inconsistent stage, this could be further improved by first backing up old files to temporary location to allow undo on failure, but leaving that for some other day. Also unowned directories will still be left behind. And yes, this is a somewhat scary change as it's the biggest ever change to how rpm lays down files on install. Adopt the hardlink test spec over to install tests and add some more tests for the new behavior. Fixes: rpm-software-management#967 (+ multiple reports over the years)
|
Rebased and fixups squashed. |
|
My concerns have been addressed. Looks really good now! |
|
Okay, lets move on then, I've a further pile building on top of this already. Thanks for reviewing. |
Run the file installation in multiple stages:
This means we no longer leave behind a trail of untracked, potentially harmful junk on installation failure.
If commit step fails the package can still be left in an inconsistent stage, this could be further improved by first backing up old files to temporary location to allow undo on failure, but leaving that for some other day.
Also unowned directories will still be left behind.
And yes, this is a somewhat scary change as it's the biggest ever change to how rpm lays down files on install.
Fixes: #967 (+ multiple reports over the years)
Details in commit messages, there are several preparatory commits leading up to the "big switch" to this new mode of operation. This change has also been the major blocker to making the plugin API public, as this can have severe consequences on plugins which only expect to handle files one by one. Our resident plugins are not actually affected though.