[pacman-dev] [PATCH] alpm: Add event ALPM_EVENT_HOOK_RUN_FAILED

[Andrew Gregory]

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