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

[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?

> 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