Re: [pacman-dev] [PATCH] redirect scriptlet stderr through alpm
Sorry, I forgot to rebase against master. Here's the real patch: 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(-) 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); } 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]); } 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); + } + + 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
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
On Fri, 2010-05-21 at 09:30 +0200, Xavier Chantry wrote:
Comments like this should be put below (look below :))
Thanks, that is good to know :).
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 ?
That was my thinking, yes.
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.
The frontend can still read the result of printf because the redirection occurs beforehand. Essentially it would just look like the scriptlet itself failed to call chroot. I haven't tested this though - do you know of a safe way to make chroot fail?
IMO it's worth a comment explaining we use printf instead of alpm_log to avoid callback in child because it can cause problems.
Ok, fair enough.
To be honest, I never looked into thread safety problems so I don't know anything about this.
I'm no expert either, but there are other reasons not to have a callback. For example, the frontend might want to cache the output of the scriptlet and display it later, which is not possible currently since the two processes have different address spaces. This kind of bug could be very confusing for the frontend writer.
+ 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 ?
I don't think so, because in that case pipefd[0] will still be open, even if it has reached EOF. Failing to open is probably impossible (the man page says it will only fail if malloc or fcntl do too), but in the unlikely case that it does, feof will cause a segfault. I made an error message for this case in my previous patch, should I put one in this one as well?
Do we need close(pipefd[0]); there too or is that done anyway ?
It is done anyway: see the man page fdopen(3). I also tested this just in case, and closing it twice does no harm, but the file descriptor is definitely closed by fclose. Thanks for your input :). Jonathan
On Fri, May 21, 2010 at 10:36 AM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
On Fri, 2010-05-21 at 09:30 +0200, Xavier Chantry wrote:
Comments like this should be put below (look below :))
Thanks, that is good to know :).
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 ?
That was my thinking, yes.
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.
The frontend can still read the result of printf because the redirection occurs beforehand. Essentially it would just look like the scriptlet itself failed to call chroot. I haven't tested this though - do you know of a safe way to make chroot fail?
Ahah of course, the bug was on my side, printf in the child makes perfect sense. I would not bother trying to make chroot fail, just suppose it fails, and make the code printf and exit(1) for a quick test.
IMO it's worth a comment explaining we use printf instead of alpm_log to avoid callback in child because it can cause problems.
Ok, fair enough.
To be honest, I never looked into thread safety problems so I don't know anything about this.
I'm no expert either, but there are other reasons not to have a callback. For example, the frontend might want to cache the output of the scriptlet and display it later, which is not possible currently since the two processes have different address spaces. This kind of bug could be very confusing for the frontend writer.
+ 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 ?
I don't think so, because in that case pipefd[0] will still be open, even if it has reached EOF. Failing to open is probably impossible (the man page says it will only fail if malloc or fcntl do too), but in the unlikely case that it does, feof will cause a segfault. I made an error message for this case in my previous patch, should I put one in this one as well?
Well I first thought it was an error path, but then why not handling it like one ? e.g. just below in the code : _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), strerror(errno)); retval = 1; goto cleanup; Now looking at the manpage, I see it fails just if/like malloc does. But we usually handle malloc/calloc failures as well in the code using MALLOC/CALLOC macros.
Do we need close(pipefd[0]); there too or is that done anyway ?
It is done anyway: see the man page fdopen(3). I also tested this just in case, and closing it twice does no harm, but the file descriptor is definitely closed by fclose.
Ok I suspected it worked that way, good to see it's mentioned in the man page.
Thanks for your input :).
Thanks for the help, it's very welcome !
On Fri, 2010-05-21 at 12:33 +0200, Xavier Chantry wrote:
I would not bother trying to make chroot fail, just suppose it fails, and make the code printf and exit(1) for a quick test.
IMO it's worth a comment explaining we use printf instead of alpm_log to avoid callback in child because it can cause problems.
I'll test this and add that...
Well I first thought it was an error path, but then why not handling it like one ? e.g. just below in the code : _alpm_log(PM_LOG_ERROR, _("call to waitpid failed (%s)\n"), strerror(errno)); retval = 1; goto cleanup;
Now looking at the manpage, I see it fails just if/like malloc does. But we usually handle malloc/calloc failures as well in the code using MALLOC/CALLOC macros.
... then put the error handling back in and resubmit. Jonathan
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 | 91 +++++++++++++++++++++++++++++++-------------------- 1 files changed, 55 insertions(+), 36 deletions(-) Xavier: I also changed the printf calls to fprintf(stderr, ...), which technically doesn't change anything but was what I did originally. Before I wasn't sure that it would be redirected properly, as 'stderr' itself might still reference the original stream, but I've tested it now and it works. diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..4f7888d 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,67 @@ 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); + while(dup2(pipefd[1], 1) == -1 && errno == EINTR); + while(dup2(pipefd[1], 2) == -1 && errno == EINTR); + close(pipefd[0]); + close(pipefd[1]); + + /* use fprintf instead of _alpm_log to send output through the parent */ 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); } 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; 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)); + FILE *pipe; + + close(pipefd[1]); + pipe = fdopen(pipefd[0], "r"); + if(pipe == NULL) { + close(pipefd[0]); retval = 1; - goto cleanup; } 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); + } + + 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 != 0) { + _alpm_log(PM_LOG_ERROR, _("could not open pipe (%s)\n"), strerror(errno)); + 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
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. Also adds a shell to the fake root set up for pactests, which was not needed previously due to a bug (debian #582847) in fakechroot. Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org> --- lib/libalpm/util.c | 91 +++++++++++++++++++++++++++++++-------------------- pactest/pmtest.py | 5 ++- 2 files changed, 59 insertions(+), 37 deletions(-) I should point out that some of this is Xavier's work and not mine. diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 32eaa44..4f7888d 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,67 @@ 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); + while(dup2(pipefd[1], 1) == -1 && errno == EINTR); + while(dup2(pipefd[1], 2) == -1 && errno == EINTR); + close(pipefd[0]); + close(pipefd[1]); + + /* use fprintf instead of _alpm_log to send output through the parent */ 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); } 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; 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)); + FILE *pipe; + + close(pipefd[1]); + pipe = fdopen(pipefd[0], "r"); + if(pipe == NULL) { + close(pipefd[0]); retval = 1; - goto cleanup; } 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); + } + + 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 != 0) { + _alpm_log(PM_LOG_ERROR, _("could not open pipe (%s)\n"), strerror(errno)); + 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; } } } diff --git a/pactest/pmtest.py b/pactest/pmtest.py index f2b9676..f3026f2 100755 --- a/pactest/pmtest.py +++ b/pactest/pmtest.py @@ -110,10 +110,13 @@ def generate(self): tmpdir = os.path.join(self.root, TMPDIR) logdir = os.path.join(self.root, os.path.dirname(LOGFILE)) etcdir = os.path.join(self.root, os.path.dirname(PACCONF)) - for dir in [dbdir, cachedir, syncdir, tmpdir, logdir, etcdir]: + bindir = os.path.join(self.root, "bin") + for dir in [dbdir, cachedir, syncdir, tmpdir, logdir, etcdir, bindir]: if not os.path.isdir(dir): vprint("\t%s" % dir[len(self.root)+1:]) os.makedirs(dir, 0755) + # Only the dynamically linked binary is needed for fakechroot + shutil.copy("/bin/sh", bindir) # Configuration file vprint(" Creating configuration file") -- 1.7.1
On Mon, May 24, 2010 at 2:19 AM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
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. Also adds a shell to the fake root set up for pactests, which was not needed previously due to a bug (debian #582847) in fakechroot.
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org>
What's the status on this patch? Andres P
On Thu, Jun 24, 2010 at 9:57 PM, Andres P <aepd87@gmail.com> wrote:
On Mon, May 24, 2010 at 2:19 AM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
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. Also adds a shell to the fake root set up for pactests, which was not needed previously due to a bug (debian #582847) in fakechroot.
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org>
What's the status on this patch?
I liked them so I pushed them to my branch : http://code.toofishes.net/cgit/xavier/pacman.git/log/?h=working I suppose Dan will look at them when he has time, and that they are not maint-worthy, so they will wait until next major release. i.e. no big hurry.
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org> --- lib/libalpm/trans.c | 3 ++- lib/libalpm/util.c | 9 +++++---- lib/libalpm/util.h | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) The _alpm_log line below (around 467,7) won't be as detailed as it was before, can anyone suggest a solution for this? diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 49fc0f6..79d2614 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -344,6 +344,7 @@ int _alpm_runscriptlet(const char *root, const char *installfn, char scriptfn[PATH_MAX]; char cmdline[PATH_MAX]; char tmpdir[PATH_MAX]; + char *argv[] = { "sh", "-c", cmdline, NULL }; char *scriptpath; int clean_tmpdir = 0; int retval = 0; @@ -401,7 +402,7 @@ int _alpm_runscriptlet(const char *root, const char *installfn, scriptpath, script, ver); } - retval = _alpm_run_chroot(root, cmdline); + retval = _alpm_run_chroot(root, "/bin/sh", argv); cleanup: if(clean_tmpdir && _alpm_rmrf(tmpdir)) { diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 4f7888d..f2c80f2 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -444,7 +444,7 @@ int _alpm_logaction(int usesyslog, FILE *f, const char *fmt, va_list args) return(ret); } -int _alpm_run_chroot(const char *root, const char *cmd) +int _alpm_run_chroot(const char *root, const char *path, char *const argv[]) { char cwd[PATH_MAX]; pid_t pid; @@ -467,7 +467,7 @@ int _alpm_run_chroot(const char *root, const char *cmd) goto cleanup; } - _alpm_log(PM_LOG_DEBUG, "executing \"%s\" under chroot \"%s\"\n", cmd, root); + _alpm_log(PM_LOG_DEBUG, "executing \"%s\" under chroot \"%s\"\n", path, root); /* Flush open fds before fork() to avoid cloning buffers */ fflush(NULL); @@ -505,7 +505,7 @@ int _alpm_run_chroot(const char *root, const char *cmd) exit(1); } umask(0022); - execl("/bin/sh", "sh", "-c", cmd, (char *) NULL); + execv(path, argv); fprintf(stderr, _("call to execl failed (%s)\n"), strerror(errno)); exit(1); } else { @@ -570,7 +570,8 @@ int _alpm_ldconfig(const char *root) if(access(line, F_OK) == 0) { snprintf(line, PATH_MAX, "%ssbin/ldconfig", root); if(access(line, X_OK) == 0) { - _alpm_run_chroot(root, "/sbin/ldconfig"); + char *argv[] = { "ldconfig", NULL }; + _alpm_run_chroot(root, "/sbin/ldconfig", argv); } } diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 8a3154a..35c4d28 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -68,7 +68,7 @@ int _alpm_unpack_single(const char *archive, const char *prefix, const char *fn) int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int breakfirst); int _alpm_rmrf(const char *path); int _alpm_logaction(int usesyslog, FILE *f, const char *fmt, va_list args); -int _alpm_run_chroot(const char *root, const char *cmd); +int _alpm_run_chroot(const char *root, const char *path, char *const argv[]); int _alpm_ldconfig(const char *root); int _alpm_str_cmp(const void *s1, const void *s2); char *_alpm_filecache_find(const char *filename); -- 1.7.1
On Mon, May 24, 2010 at 8:49 AM, Jonathan Conder <j@skurvy.no-ip.org> wrote:
Signed-off-by: Jonathan Conder <j@skurvy.no-ip.org> --- lib/libalpm/trans.c | 3 ++- lib/libalpm/util.c | 9 +++++---- lib/libalpm/util.h | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-)
The _alpm_log line below (around 467,7) won't be as detailed as it was before, can anyone suggest a solution for this?
IMO that log line was not very informative in the first place as it showed the temporary scriptlet file which only exists at runtime and is deleted right after being used. BTW we are not constrained to log from run_chroot at all, we could actually have a more informative debug line by logging (inside) alpm_runscriptlet and its arguments. Otherwise the two patches look good to me, thanks !
participants (3)
-
Andres P
-
Jonathan Conder
-
Xavier Chantry