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