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

[Andrew Gregory]

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.

> 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.

> > 
> > >  lib/libalpm/alpm.h    | 6 +++++-
> > >  lib/libalpm/hook.c    | 7 +++++--
> > >  src/pacman/callback.c | 4 ++++
> > >  3 files changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > > index f6e6b86..52cbd51 100644
> > > --- a/lib/libalpm/alpm.h
> > > +++ b/lib/libalpm/alpm.h
> > > @@ -465,7 +465,9 @@ typedef enum _alpm_event_type_t {
> > >  	/** A hook is starting */
> > >  	ALPM_EVENT_HOOK_RUN_START,
> > >  	/** A hook has finnished runnning */
> > > -	ALPM_EVENT_HOOK_RUN_DONE
> > > +	ALPM_EVENT_HOOK_RUN_DONE,
> > > +	/** A hook has failed to run. */
> > > +	ALPM_EVENT_HOOK_RUN_FAILED
> > >  } alpm_event_type_t;
> > >  
> > >  typedef struct _alpm_event_any_t {
> > > @@ -568,6 +570,8 @@ typedef struct _alpm_event_hook_run_t {
> > >  	alpm_event_type_t type;
> > >  	/** Text to be outputted */
> > >  	const char *desc;
> > > +	/** Whether the hook has the AbortOnFail flag set
> > > (pre-transaction only) */  
> > 
> > This is not strictly true.  Setting AbortOnFail on a PostTransaction
> > hook generates a warning, but leaves the flag set.  In that case your
> > callback will falsely claim that it aborted the transaction.
> 
> Oh, I missed that. Easy enough to set the flag here only for
> pre-transaction hooks though. (Since it doesn't apply, why are
> abort_on_fail flags set for post-transaction hooks though?)

At the time abort_on_fail is set we don't know if it's a pre or post
hook.  We would have to *unset* the flag in hook_validate, which
I didn't do at the time because there was no reason to.