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:
On Mon, 14 Dec 2015 12:47:26 -0500 Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
On 12/14/15 at 05:55pm, Olivier Brunel wrote:
Also include the abort_on_fail flag so the frontend can let the user know which hook(s) are responsible.
Signed-off-by: Olivier Brunel <jjk@jjacky.com> ---
This seems like overkill. A failed AbortOnFail hook will already generate 3 error messages (assuming the hook itself prints one). We could just label AbortOnFail hooks as such when we print the info during HOOK_RUN_START. Alternatively, and this might be preferable behavior anyway, we could simply stop running PreTransaction hooks as soon as an AbortOnFail hook fails.
But there's no guarantee the hook will actually print anything, so it is good for frontends to (be able to) do so. And this is the only way to tell which hook(s) are causing the transaction to be aborted, so I don't see this as overkill.
Even if the hook itself doesn't print anything, libalpm will always print a message if a scriptlet or hook doesn't return 0. I'm fine with allowing front-ends to mark failed hooks as being AbortOnFail, I just don't think justifies adding a whole new event type.
Sure libalpm will print an error message (w/out the hook name), but in the case where multiple hooks are run, and multiple hooks can fail, there would be no way to know which one(s) are actually causing the aborted transaction and which ones are not. And without this new event, frontends have no way of knowing whether a hook run was successful or not. All they know is it ran (ALPM_EVENT_HOOK_RUN_DONE), without any further indication.
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.