On Sat, May 15, 2010 at 5:41 PM, Jonathan Conder <[email protected]> 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 <[email protected]>
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 ?
From aee35a61dd2b19db869acb9f30df8d27435e45b4 Mon Sep 17 00:00:00 2001 From: Xavier Chantry <[email protected]> Date: Sun, 15 Mar 2009 17:37:36 +0100 Subject: [PATCH] runscriptlet : switch back from popen to execl commit ad691001e20272b794d2ed574b556f520e3555c0 simplified a lot the execution of scriptlets, and in particular switched from popen to execl. However, we lost scriptlet logging here. So commit 006387828cbdd11e6307879ad27e9bb9409ca193 switched back to popen for reading the output and logging it. However, we can keep using execl (maybe this spares one fork?) and still get the output with a simple pipe and redirection. Signed-off-by: Xavier Chantry <[email protected]> --- lib/libalpm/trans.c | 35 +++++++++++++++++++++-------------- 1 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index bf4a3eb..340e8c0 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -440,6 +440,7 @@ int _alpm_runscriptlet(const char *root, const char *installfn, char cwd[PATH_MAX]; char *scriptpath; pid_t pid; + int pipefd[2]; int clean_tmpdir = 0; int restore_cwd = 0; int retval = 0; @@ -516,6 +517,11 @@ int _alpm_runscriptlet(const char *root, const char *installfn, /* Flush open fds before fork() to avoid cloning buffers */ fflush(NULL); + if(pipe(pipefd) == -1) { + _alpm_log(PM_LOG_ERROR, _("could not create pipe (%s)\n"), strerror(errno)); + retval = 1; + goto cleanup; + } /* fork- parent and child each have seperate code blocks below */ pid = fork(); if(pid == -1) { @@ -525,7 +531,6 @@ int _alpm_runscriptlet(const char *root, const char *installfn, } 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); if(chroot(root) != 0) { @@ -540,13 +545,18 @@ int _alpm_runscriptlet(const char *root, const char *installfn, } umask(0022); _alpm_log(PM_LOG_DEBUG, "executing \"%s\"\n", cmdline); - /* execl("/bin/sh", "sh", "-c", cmdline, (char *)NULL); */ - pipe = popen(cmdline, "r"); - if(!pipe) { - _alpm_log(PM_LOG_ERROR, _("call to popen failed (%s)"), - strerror(errno)); - exit(1); - } + close(1); + dup(pipefd[1]); + close(pipefd[0]); + execl("/bin/sh", "sh", "-c", cmdline, (char *)NULL); + } else { + /* this code runs for the parent only (wait on the child) */ + pid_t retpid; + int status; + FILE *pipe; + + close(pipefd[1]); + pipe = fdopen(pipefd[0], "r"); while(!feof(pipe)) { char line[PATH_MAX]; if(fgets(line, PATH_MAX, pipe) == NULL) @@ -554,12 +564,9 @@ int _alpm_runscriptlet(const char *root, const char *installfn, alpm_logaction("%s", line); EVENT(trans, PM_TRANS_EVT_SCRIPTLET_INFO, line, NULL); } - retval = pclose(pipe); - exit(WEXITSTATUS(retval)); - } else { - /* this code runs for the parent only (wait on the child) */ - pid_t retpid; - int status; + fclose(pipe); + close(pipefd[0]); + while((retpid = waitpid(pid, &status, 0)) == -1 && errno == EINTR); if(retpid == -1) { _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), -- 1.7.1
