[bug#56218] [PATCH] guix: inferior: Fix the behaviour of open-inferior #:error-port.

["Ludovic Courtès" <ludo@gnu.org>]

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> 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.

OK, I think I got it.

>>> +            (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))

I prefer the original version: I find it clearer (it’s low-level) and
probably more robust (thinking through the port/FD interaction needs is
more demanding :-)).

>> 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.

To sum up: I think it’s a welcome change, and it’s even more welcome
with a couple of tests to make sure it behaves the way we think it does.

Thanks!

Ludo’.