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

[Xavier Chantry]

On Fri, May 21, 2010 at 10:36 AM, Jonathan Conder <j at 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 !