[pacman-dev] [PATCH] alpm: Abort ASAP on failure in pre-transaction hooks
There is no need to run all/remaining pre-transaction hooks as soon as a failure has occured, which will lead to aborting the transaction. So as soon as an error in the first phase (reading directories/parsing files) occurs, or a hook flagged abort_on_fail does fail, we stop processing and return. (For post-transaction hooks, all hooks are run regardless since there's no aborting.) Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- As discussed earlier with Andrew, this "replaces" the previous "alpm: Add event ALPM_EVENT_HOOK_RUN_FAILED" patch I sent. lib/libalpm/hook.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index acd571d..9b204fb 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -624,6 +624,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) struct dirent entry, *result; DIR *d; + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } + if((dirlen = strlen(i->data)) >= PATH_MAX) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not open directory: %s: %s\n"), (char *)i->data, strerror(ENAMETOOLONG)); @@ -648,6 +652,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) struct stat buf; size_t name_len; + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } + if(strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { continue; } @@ -708,6 +716,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) closedir(d); } + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + goto cleanup; + } + hooks = alpm_list_msort(hooks, alpm_list_count(hooks), (alpm_list_fn_cmp)_alpm_hook_cmp); @@ -743,6 +755,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) hook_event.type = ALPM_EVENT_HOOK_RUN_DONE; EVENT(handle, &hook_event); + + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } } event.type = ALPM_EVENT_HOOK_DONE; -- 2.6.4
On 12/15/15 at 12:07am, Olivier Brunel wrote:
There is no need to run all/remaining pre-transaction hooks as soon as a failure has occured, which will lead to aborting the transaction.
So as soon as an error in the first phase (reading directories/parsing files) occurs, or a hook flagged abort_on_fail does fail, we stop processing and return.
(For post-transaction hooks, all hooks are run regardless since there's no aborting.)
Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- As discussed earlier with Andrew, this "replaces" the previous "alpm: Add event ALPM_EVENT_HOOK_RUN_FAILED" patch I sent.
Remove the first two checks that break out of the parsing loop and I'm good with this. Going ahead with parsing after errors will allow us to give the user all parsing errors at once instead of only providing a single error and potentially forcing the user into a run pacman, fix hook, run pacman, fix next hook, ... cycle.
lib/libalpm/hook.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index acd571d..9b204fb 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -624,6 +624,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) struct dirent entry, *result; DIR *d;
+ if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } + if((dirlen = strlen(i->data)) >= PATH_MAX) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not open directory: %s: %s\n"), (char *)i->data, strerror(ENAMETOOLONG)); @@ -648,6 +652,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) struct stat buf; size_t name_len;
+ if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } + if(strcmp(entry.d_name, ".") == 0 || strcmp(entry.d_name, "..") == 0) { continue; } @@ -708,6 +716,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) closedir(d); }
+ if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + goto cleanup; + } + hooks = alpm_list_msort(hooks, alpm_list_count(hooks), (alpm_list_fn_cmp)_alpm_hook_cmp);
@@ -743,6 +755,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
hook_event.type = ALPM_EVENT_HOOK_RUN_DONE; EVENT(handle, &hook_event); + + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } }
event.type = ALPM_EVENT_HOOK_DONE; -- 2.6.4
There is no need to run any/remaining pre-transaction hooks as soon as a failure has occured, which will lead to aborting the transaction. So if an error occured during the first phase (reading directories/parsing files), or as soon as a hook flagged abort_on_fail does fail, we stop processing them and return. (For post-transaction hooks, all hooks are run regardless since there's no aborting.) v2: Don't stop during first phase, so e.g. all parsing errors can be found & reported at once. Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/hook.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index acd571d..74d7a5b 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -708,6 +708,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) closedir(d); } + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + goto cleanup; + } + hooks = alpm_list_msort(hooks, alpm_list_count(hooks), (alpm_list_fn_cmp)_alpm_hook_cmp); @@ -743,6 +747,10 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) hook_event.type = ALPM_EVENT_HOOK_RUN_DONE; EVENT(handle, &hook_event); + + if(ret != 0 && when == ALPM_HOOK_PRE_TRANSACTION) { + break; + } } event.type = ALPM_EVENT_HOOK_DONE; -- 2.6.4
On 12/15/15 at 09:14am, Olivier Brunel wrote:
There is no need to run any/remaining pre-transaction hooks as soon as a failure has occured, which will lead to aborting the transaction.
So if an error occured during the first phase (reading directories/parsing files), or as soon as a hook flagged abort_on_fail does fail, we stop processing them and return.
(For post-transaction hooks, all hooks are run regardless since there's no aborting.)
v2: Don't stop during first phase, so e.g. all parsing errors can be found & reported at once.
Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/hook.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Ack. The v2 note just needs to be removed from the commit message when this gets merged.
participants (2)
-
Andrew Gregory
-
Olivier Brunel