On 05/23/20 at 02:04pm, Daan De Meyer wrote:
This makes debugging hook failures when running pacman less of a hassle. Instead of a generic "command failed to execute correctly", we now get the exact command and its arguments, as well as the exit code of the command logged.
Signed-off-by: Daan De Meyer <daan.j.demeyer@gmail.com> ---
Updated strv_join failure handling. I'm also not entirely sure what to do with the translations. Do I update all the separate files with the updated message identifier? I can fix some of the translations but the Arabic ones are impossible for me to fix.
No, translations are done separately through transifex.
lib/libalpm/util.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index ead03004..22ae48f6 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -568,6 +568,42 @@ static void _alpm_reset_signals(void) } }
+static char *strv_join(char *const *strv, const char *separator) +{ + char *const *i; + char *r, *p; + size_t size = 0; + + if (!separator) { + separator = " "; + } + + for (i = strv; *i; i++) { + if (i != strv) { + size += strlen(separator); + } + + size += strlen(*i); + } + + r = malloc(size + 1); + if (r == NULL) { + return NULL; + } + + p = r; + + for (i = strv; *i; i++) { + if (i != strv) { + p = stpcpy(p, separator); + } + + p = stpcpy(p, *i); + } + + return r; +}
We already have an almost identical function in pacman/util.c.
/** Execute a command with arguments in a chroot. * @param handle the context handle * @param cmd command to execute @@ -745,7 +781,14 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[], if(WIFEXITED(status)) { _alpm_log(handle, ALPM_LOG_DEBUG, "call to waitpid succeeded\n"); if(WEXITSTATUS(status) != 0) { - _alpm_log(handle, ALPM_LOG_ERROR, _("command failed to execute correctly\n")); + char *argv_str = strv_join(argv, NULL); + if (argv_str) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("command \"%s\" failed to execute correctly: %i\n"), + argv_str, WEXITSTATUS(status));
I doubt that the exit status is generally going to be useful to the user, nor is it clear from the error message that that's even what is being printed.
+ free(argv_str); + }
I don't know that this is the right approach. This is going to give pretty useless, and probably confusing, output for scriptlets. Hooks and scripts should really be printing appropriate error messages when they fail, so that our error message should really only matter in the rare occasions where the command fails to execute at all. I would be more inclined to point the user at the actual hook or script that failed.
+ retval = 1; } } else if(WIFSIGNALED(status) != 0) { -- 2.26.2