This looks mostly good to me, I like this patch as a first smaller step, thanks ! Just have a few comments below, and I didn't test it yet. On Fri, May 21, 2010 at 5:22 AM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
Sorry, I forgot to rebase against master. Here's the real patch:
Comments like this should be put below (look below :))
Fixes FS#18770, and hopefully an occasional deadlock in my frontend as well. For simplicity it redirects all scriptlet output through SCRIPTLET_INFO, and all callbacks in the child process have been replaced for thread-safety.
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org> --- lib/libalpm/util.c | 86 +++++++++++++++++++++++++++++---------------------- 1 files changed, 49 insertions(+), 37 deletions(-)
Here between --- and diff, you can put any comments that don't belong to commit log / git history.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..62851c1 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -448,6 +448,7 @@ int _alpm_run_chroot(const char *root, const char *cmd) { char cwd[PATH_MAX]; pid_t pid; + int pipefd[2]; int restore_cwd = 0; int retval = 0;
@@ -471,6 +472,12 @@ int _alpm_run_chroot(const char *root, const char *cmd) /* 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) { @@ -480,55 +487,60 @@ 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(1); + close(2); + close(pipefd[0]); + while(dup2(pipefd[1], 1) == -1 && errno == EINTR); + while(dup2(pipefd[1], 2) == -1 && errno == EINTR); + close(pipefd[1]); + if(chroot(root) != 0) { - _alpm_log(PM_LOG_ERROR, _("could not change the root directory (%s)\n"), - strerror(errno)); + printf(_("could not change the root directory (%s)\n"), strerror(errno)); exit(1); }
So we want to avoid callback here and cannot use alpm_log in the child ? A printf in libalpm does not seem very useful for a frontend. But well maybe we would keep it for easier debugging of pacman. And at least frontend will see the return value. IMO it's worth a comment explaining we use printf instead of alpm_log to avoid callback in child because it can cause problems. To be honest, I never looked into thread safety problems so I don't know anything about this.
if(chdir("/") != 0) { - _alpm_log(PM_LOG_ERROR, _("could not change directory to / (%s)\n"), - strerror(errno)); + printf(_("could not change directory to / (%s)\n"), strerror(errno)); exit(1); } 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); + printf(_("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; 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)); - retval = 1; - goto cleanup; + FILE *pipe; + + close(pipefd[1]); + pipe = fdopen(pipefd[0], "r"); + if(pipe == NULL) { + close(pipefd[0]);
This is not a failure, we reach that for a scriptlet with no output ? If so, can you put a quick comment for that case ?
} 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(!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); + } + fclose(pipe);
Do we need close(pipefd[0]); there too or is that done anyway ?
+ } + + 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; + } + } + + /* 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; } } } -- 1.7.1