On 12/14/15 at 08:30pm, Olivier Brunel wrote:
On Mon, 14 Dec 2015 13:20:05 -0500 Andrew Gregory <andrew.gregory.8@gmail.com> 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