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 !