Ludovic Courtès <ludo@gnu.org> writes:

> Hi Christopher,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> This should be the error port used by the inferior process, but currently it's
>> either stderr if #:error-port is a file port, or /dev/null otherwise.
>
> That’s still the case with this patch, no?
>
> The patch does make a difference when (current-error-port) wraps a file
> descriptor other than 2 though.

Maybe this sentance is a little unclear.

What I'm trying to say is that passing a port as #:error-port doesn't
really work. There's no scenario where the output actually goes to the
port you provide, though it can have some effect.

>> +++ b/guix/inferior.scm
>> @@ -156,12 +156,14 @@ (define (open-bidirectional-pipe command . args)
>>              (close-port parent)
>>              (close-fdes 0)
>>              (close-fdes 1)
>> +            (close-fdes 2)
>>              (dup2 (fileno child) 0)
>>              (dup2 (fileno child) 1)
>>              ;; Mimic 'open-pipe*'.
>> -            (unless (file-port? (current-error-port))
>> -              (close-fdes 2)
>> -              (dup2 (open-fdes "/dev/null" O_WRONLY) 2))
>> +            (dup2 (if (file-port? (current-error-port))
>> +                      (fileno (current-error-port))
>> +                      (open-fdes "/dev/null" O_WRONLY))
>> +                  2)
>
> If (current-error-port) wraps FD 2 when the function is called, then, by
> the time we reach (dup2 … 2), the FD behind (current-error-port) has be
> closed; we end up doing (dup2 2 2), but FD 2 is closed, so we get EBADF.
>
> Or am I misunderstanding?

That sounds reasonable, I've only tested this change in the scenario
when the #:error-port isn't stderr, and I mostly adapted this from what
I thought open-pipe* did.

Maxime suggested using move->fdes, so maybe this would be an improved
version:

  ;; Mimic 'open-pipe*'.
  (if (file-port? (current-error-port))
      (unless (eq? (fileno (current-error-port)) 2)
        (move-fdes (current-error-port) 2))
      (move->fdes (open-file "/dev/null" O_WRONLY) 2))

> Perhaps we should add one test for each case (error port is a file port
> vs. error port is another kind of port) in ‘tests/inferior.scm’.

Yep, sounds good.

Thanks,

Chris