[pacman-dev] [PATCH] redirect scriptlet stderr through alpm

[Xavier Chantry]

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 at 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 at 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
>
>
>
>