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 <[email protected]> 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 <[email protected]> > --- > 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 > > > >
