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
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
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
--- 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
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
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
Signed-off-by: Jonathan Conder
On Mon, May 24, 2010 at 8:49 AM, Jonathan Conder
Signed-off-by: Jonathan Conder
--- 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 !
On Mon, May 24, 2010 at 2:19 AM, Jonathan Conder
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
What's the status on this patch? Andres P
On Thu, Jun 24, 2010 at 9:57 PM, Andres P
On Mon, May 24, 2010 at 2:19 AM, Jonathan Conder
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
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.
participants (3)
-
Andres P
-
Jonathan Conder
-
Xavier Chantry