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