[pacman-dev] [PATCH] redirect scriptlet stderr through alpm
Fixes FS#18770, and hopefully an occasional deadlock in my frontend as well. This introduces a new EVENT: PM_TRANS_EVT_SCRIPTLET_ERROR, and moves all scriptlet-related callbacks into the parent process. I modelled my work on the GLib function g_spawn_sync, but I am quite unfamiliar with POSIX so there could be some issues with the implementation. However, all the tests I have performed so far (against v3.3.3) seemed to work fine (even the kernel installed and gave me the right warnings). This patch might also make it easier to configure the shell used for install scriptlets. I'm not 100% happy with the code structure at the moment, so any suggestions would be welcome. Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org> --- lib/libalpm/alpm.h | 4 + lib/libalpm/util.c | 162 ++++++++++++++++++++++++++++++++++++++----------- src/pacman/callback.c | 3 + 3 files changed, 133 insertions(+), 36 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 3329132..2692777 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -364,6 +364,10 @@ typedef enum _pmtransevt_t { * A line of text is passed to the callback. */ PM_TRANS_EVT_SCRIPTLET_INFO, + /** Scriptlet has printed information to stderr. + * A line of text is passed to the callback. + */ + PM_TRANS_EVT_SCRIPTLET_ERROR, /** Files will be downloaded from a repository. * The repository's tree name is passed to the callback. */ diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..1f76372 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -444,10 +444,52 @@ int _alpm_logaction(int usesyslog, FILE *f, const char *fmt, va_list args) return(ret); } +static int _fddup2(int fds[2], int i, int fd) +{ + while(dup2(fds[i], fd) == -1) { + if(errno != EINTR) { + return(-1); + } + } + return(0); +} + +static int _fdselect(int fdout, int fderr, fd_set *fds) +{ + int nfds = -1; + FD_ZERO(fds); + + if(fdout >= 0) { + FD_SET(fdout, fds); + if(fdout > nfds) { + nfds = fdout; + } + } + if(fderr >= 0) { + FD_SET(fderr, fds); + if(fderr > nfds) { + nfds = fderr; + } + } + if(++nfds == 0) { + return(2); + } + + if(select(nfds, fds, NULL, NULL, NULL) == -1) { + if(errno == EINTR) { + FD_ZERO(fds); + } else { + return(1); + } + } + return(0); +} + int _alpm_run_chroot(const char *root, const char *cmd) { char cwd[PATH_MAX]; pid_t pid; + int fdout[2], fderr[2]; int restore_cwd = 0; int retval = 0; @@ -471,6 +513,13 @@ int _alpm_run_chroot(const char *root, const char *cmd) /* Flush open fds before fork() to avoid cloning buffers */ fflush(NULL); + /* Create pipes to redirect stderr and stdout */ + if(pipe(fdout) == -1 || pipe(fderr) == -1) { + _alpm_log(PM_LOG_ERROR, _("could not create a pipe (%s)\n"), strerror(errno)); + retval = 1; + goto cleanup; + } + /* fork- parent and child each have seperate code blocks below */ pid = fork(); if(pid == -1) { @@ -480,56 +529,97 @@ int _alpm_run_chroot(const char *root, const char *cmd) } if(pid == 0) { - FILE *pipe; /* this code runs for the child only (the actual chroot/exec) */ - _alpm_log(PM_LOG_DEBUG, "chrooting in %s\n", root); + close(fdout[0]); + close(fderr[0]); + + /* redirect standard output and error */ + _fddup2(fdout, 1, 1); + _fddup2(fderr, 1, 2); + close(fdout[1]); + close(fderr[1]); + if(chroot(root) != 0) { - _alpm_log(PM_LOG_ERROR, _("could not change the root directory (%s)\n"), - strerror(errno)); + fprintf(stderr, _("could not change the root directory (%s)\n"), strerror(errno)); exit(1); } + if(chdir("/") != 0) { - _alpm_log(PM_LOG_ERROR, _("could not change directory to / (%s)\n"), - strerror(errno)); + fprintf(stderr, _("could not change directory to / (%s)\n"), strerror(errno)); exit(1); } + + /* run the scriptlet and exit */ umask(0022); - pipe = popen(cmd, "r"); - if(!pipe) { - _alpm_log(PM_LOG_ERROR, _("call to popen failed (%s)\n"), - strerror(errno)); - exit(1); - } - while(!feof(pipe)) { - char line[PATH_MAX]; - if(fgets(line, PATH_MAX, pipe) == NULL) - break; - alpm_logaction("%s", line); - EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL); - } - retval = pclose(pipe); - exit(WEXITSTATUS(retval)); + execl("/bin/sh", "sh", "-c", cmd, (char *) NULL); + fprintf(stderr, _("call to execl failed (%s)\n"), strerror(errno)); + exit(1); } else { - /* this code runs for the parent only (wait on the child) */ - pid_t retpid; + /* this code runs for the parent only (process output from the child) */ + fd_set fds; + FILE *fout, *ferr; int status; - do { - retpid = waitpid(pid, &status, 0); - } while(retpid == -1 && errno == EINTR); - if(retpid == -1) { - _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), - strerror(errno)); + + close(fdout[1]); + close(fderr[1]); + + /* use buffered streams to read line by line */ + if((fout = fdopen(fdout[0], "r")) == NULL || (ferr = fdopen(fderr[0], "r")) == NULL) { retval = 1; - goto cleanup; + close(fdout[0]); + close(fderr[0]); } else { - /* check the return status, make sure it is 0 (success) */ - if(WIFEXITED(status)) { - _alpm_log(PM_LOG_DEBUG, "call to waitpid succeeded\n"); - if(WEXITSTATUS(status) != 0) { - _alpm_log(PM_LOG_ERROR, _("command failed to execute correctly\n")); - retval = 1; + while((retval = _fdselect(fdout[0], fderr[0], &fds)) == 0) { + char line[PATH_MAX]; + + if(FD_ISSET(fdout[0], &fds)) { + if(fgets(line, PATH_MAX, fout) != NULL) { + alpm_logaction("%s", line); + EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL); + } else { + fdout[0] = -1; + } + } + + if(FD_ISSET(fderr[0], &fds)) { + if(fgets(line, PATH_MAX, ferr) != NULL) { + alpm_logaction("%s", line); + EVENT(handle->trans, PM_TRANS_EVT_SCRIPTLET_ERROR, line, NULL); + } else { + fderr[0] = -1; + } } } + + fclose(fout); + fclose(ferr); + } + + while(waitpid(pid, &status, 0) == -1) { + if (errno != EINTR) { + _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), + strerror(errno)); + retval = 1; + goto cleanup; + } + } + + /* report error from above after the child has exited */ + if(retval == 1) { + _alpm_log(PM_LOG_ERROR, _("could not read from pipe (%s)\n"), + strerror(errno)); + goto cleanup; + } else { + retval = 0; + } + + /* check the return status, make sure it is 0 (success) */ + if(WIFEXITED(status)) { + _alpm_log(PM_LOG_DEBUG, "call to waitpid succeeded\n"); + if(WEXITSTATUS(status) != 0) { + _alpm_log(PM_LOG_ERROR, _("command failed to execute correctly\n")); + retval = 1; + } } } diff --git a/src/pacman/callback.c b/src/pacman/callback.c index f5bf17d..a33178e 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -226,6 +226,9 @@ void cb_trans_evt(pmtransevt_t event, void *data1, void *data2) case PM_TRANS_EVT_SCRIPTLET_INFO: printf("%s", (char*)data1); break; + case PM_TRANS_EVT_SCRIPTLET_ERROR: + fprintf(stderr, "%s", (char*)data1); + break; case PM_TRANS_EVT_RETRIEVE_START: printf(_(":: Retrieving packages from %s...\n"), (char*)data1); break; -- 1.7.1
On Sat, May 15, 2010 at 5:41 PM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
Fixes FS#18770, and hopefully an occasional deadlock in my frontend as well. This introduces a new EVENT: PM_TRANS_EVT_SCRIPTLET_ERROR, and moves all scriptlet-related callbacks into the parent process.
I modelled my work on the GLib function g_spawn_sync, but I am quite unfamiliar with POSIX so there could be some issues with the implementation. However, all the tests I have performed so far (against v3.3.3) seemed to work fine (even the kernel installed and gave me the right warnings). This patch might also make it easier to configure the shell used for install scriptlets. I'm not 100% happy with the code structure at the moment, so any suggestions would be welcome.
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org>
Uhm I wonder how I managed to assign that bug to myself, whether it was on purpose or by mistake. I doubt I looked at it more than 5 seconds, since I couldn't remember that (fairly recent) bug at all . I had a much simpler patch lying around to just switch back from popen to execl, but since it just added complexity with no gain, I just let it die. I just attach it for reference, but its not important. Reading the bug again, I agree something must be done, but I am completely undecided which path to take. The easy way sounds attractive. So we should discuss a bit about how important that is : "stdout and stderr would be indistinguishable to the frontend". It sounds fine to distinguish both and might be the cleanest solution, but I have two questions : 1) Do applications (in particular the ones used in scriptlets) have a consistent usage of stdout and stderr ? 2) How would frontend display / handle the stdout and stderr output ? If all frontend just end up doing like pacman, displaying/mixing both output together, then I wonder if we need to bother with all this. But if you also have other justifications, that could be even more convincing. Can you elaborate on the occasional deadlock you get and how it is fixed ?
On Sun, 2010-05-16 at 14:02 +0200, Xavier Chantry wrote:
It sounds fine to distinguish both and might be the cleanest solution, but I have two questions : 1) Do applications (in particular the ones used in scriptlets) have a consistent usage of stdout and stderr ? 2) How would frontend display / handle the stdout and stderr output ?
1) I've never seen a script that uses stderr explicitly, but commands that get called use it when something goes wrong. 2) My frontend doesn't make a distinction, although I am limited by the API of PackageKit. I can't speak for other frontends though.
If all frontend just end up doing like pacman, displaying/mixing both output together, then I wonder if we need to bother with all this.
Maybe you're right, and it would probably make sense if someone wanted to write a log of pacman output, they could use > instead of &>. However, Allan seemed to imply that this wasn't the best solution, although I think now I might have misread that. See http://mailman.archlinux.org/pipermail/pacman-dev/2010-April/010618.html.
But if you also have other justifications, that could be even more convincing. Can you elaborate on the occasional deadlock you get and how it is fixed ?
I haven't been able to reproduce it since it happened last, but in my experience that usually indicates a race condition of some sort. However, I do know it occurred somewhere inside my PM_TRANS_EVT_SCRIPTLET_INFO callback, which makes a D-Bus call to pass the message to the GUI. I suspect that the problem is something to do with that, because dbus-glib is not thread-safe, let alone fork-safe. In any case I think it is more reliable to call all callbacks (including logging) in the parent process. Jonathan
On Sun, May 16, 2010 at 10:39 PM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
Maybe you're right, and it would probably make sense if someone wanted to write a log of pacman output, they could use > instead of &>. However, Allan seemed to imply that this wasn't the best solution, although I think now I might have misread that. See http://mailman.archlinux.org/pipermail/pacman-dev/2010-April/010618.html.
I am pretty sure there was no hidden meaning in what Allan wrote, i.e. no preference for the simple or the complex solution. Just that even for simple solutions, it's always nice to have patches :)
On Mon, 2010-05-17 at 00:50 +0200, Xavier Chantry wrote:
I am pretty sure there was no hidden meaning in what Allan wrote, i.e. no preference for the simple or the complex solution. Just that even for simple solutions, it's always nice to have patches :)
Ok, in that case I suggest combining the best bits our two patches, which I will do when I am less busy unless you want to do it yourself. Jonathan
participants (2)
-
Jonathan Conder
-
Xavier Chantry