On 12/14/15 at 08:30pm, Olivier Brunel wrote: > On Mon, 14 Dec 2015 13:20:05 -0500 > Andrew Gregory <[email protected]> wrote: > > > On 12/14/15 at 07:03pm, Olivier Brunel wrote: ... > > > Now I assumed there was a reason for remaining hooks to still be > > > ran, but if the behavior is to be changed to stop processing hooks > > > right away then it might not be needed indeed, since the faulty > > > hook is always the last one... > > > > I don't think I had a specific reason for continuing to run hooks > > other than not having a reason to stop running them at the time. For > > certain hooks (e.g. a btrfs snapshot) I would think immediately > > stopping would be preferable though. > > Alright, so let's drop this patch, and instead I'll send one that stops > processing pre-transaction hooks as soon as one with abort_on_fail does > fail. Sounds good? > > > Speaking of which, looking into this I realized something that I think > may be a bug/not expected behavior, though I'm not sure: > > in trans.c when running pre-transaction hooks, this is how it's done: > > if(_alpm_hook_run(handle, ALPM_HOOK_PRE_TRANSACTION) != 0) { > RET_ERR(handle, ALPM_ERR_TRANS_HOOK_FAILED, -1); > } > > Which (to me) seems to imply that it will abort if _alpm_hook_run() > doesn't return 0, and that would be classified as a hook (w/ > AbortOnFail) did fail (though the error message is more generic: > "failed to run transaction hooks"). > However, in _alpm_hook_run() there are quite a few more cases that > could lead to returning -1, e.g. failure to open a directory, to parse > a hook file, etc > > What's the intended behavior here? When failing to parse a > hook file or any of those other possible cases, should (in the case of > pre-transaction hooks) the transaction be aborted or not? > > Because if not, there should be a specific return code to trigger the > abort, distinguishing from other "non-fatal" errors to be silently > ignored; And if so, there is no point running any hooks at all then.
Aborting on any error is intentional. Marking a hook as AbortOnFail is intended to guarantee that the hook runs prior to transactions. If we fail to open a directory or parse a hook we have no way of knowing if there were any AbortOnFail hooks that we failed to run, so we take the conservative route and abort just in case. apg
