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

[Olivier Brunel]

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:
> > On Mon, 14 Dec 2015 12:47:26 -0500
> > Andrew Gregory <andrew.gregory.8 at 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 at 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.