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

Previous Next

Package: guix-patches;

Reported by: Christopher Baines <mail <at> cbaines.net>

Date: Sat, 25 Jun 2022 17:19:02 UTC

Severity: normal

Tags: patch

Done: Christopher Baines <mail <at> cbaines.net>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56218 in the body.
You can then email your comments to 56218 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Sat, 25 Jun 2022 17:19:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Christopher Baines <mail <at> cbaines.net>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sat, 25 Jun 2022 17:19:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] guix: inferior: Fix the behaviour of open-inferior
 #:error-port.
Date: Sat, 25 Jun 2022 18:18:47 +0100
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.

I'm looking at this as the Guix Data Service uses this behaviour to record and
display logs from inferior processes.

* guix/inferior.scm (open-bidirectional-pipe): Call dup2 for file descriptor
2, passing either the file number for the current error port, or a file
descriptor for /dev/null.
---
 guix/inferior.scm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/guix/inferior.scm b/guix/inferior.scm
index 54200b75e4..e36806ac84 100644
--- a/guix/inferior.scm
+++ 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)
             (apply execlp command command args))
           (lambda ()
             (primitive-_exit 127))))
-- 
2.36.1





Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Sat, 25 Jun 2022 17:51:01 GMT) Full text and rfc822 format available.

Message #8 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Christopher Baines <mail <at> cbaines.net>, 56218 <at> debbugs.gnu.org
Subject: Re: [bug#56218] [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Sat, 25 Jun 2022 19:50:12 +0200
[Message part 1 (text/plain, inline)]
Christopher Baines schreef op za 25-06-2022 om 18:18 [+0100]:
>              (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)

I don't this would work if (current-error-port) has fd 1. Would
move->fdes be appropriate here?  The following seems less fragile (*)
to me (untested, also I didn't look at the context)

  (move->fdes [child port] 0)
  (move->fdes (dup [child port]) 1)
  (if (file-port? (current-error-port))
      (move->fdes (current-error-port) 2)
      (move->fdes (open-file "/dev/null" O_WRONLY) 2))


(*): move->fdes automatically moves ports out of the way.  Also, if one
of the moves fails, then at least (current-output-port) etc will still
have a correct fd so some error reporting should be possible

Greetings,
Maxime.
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Sat, 25 Jun 2022 21:00:02 GMT) Full text and rfc822 format available.

Message #11 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Christopher Baines <mail <at> cbaines.net>, 56218 <at> debbugs.gnu.org
Subject: Re: [bug#56218] [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Sat, 25 Jun 2022 22:59:37 +0200
[Message part 1 (text/plain, inline)]
Maxime Devos schreef op za 25-06-2022 om 19:50 [+0200]:
> I don't this would work if (current-error-port) has fd 1. [...]

TBC, I never worked with file descriptor manipulation and process
forking much, and I didn't look at the surrounding code, so don't take
my word for it.

[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Sun, 26 Jun 2022 15:48:02 GMT) Full text and rfc822 format available.

Message #14 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 56218 <at> debbugs.gnu.org
Subject: Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Sun, 26 Jun 2022 17:46:57 +0200
Hi Christopher,

Christopher Baines <mail <at> 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.

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

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

Thanks,
Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Mon, 27 Jun 2022 11:41:02 GMT) Full text and rfc822 format available.

Message #17 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 56218 <at> debbugs.gnu.org
Subject: Re: [bug#56218] [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Mon, 27 Jun 2022 12:37:58 +0100
[Message part 1 (text/plain, inline)]
Maxime Devos <maximedevos <at> telenet.be> writes:

> [[PGP Signed Part:Undecided]]
> Christopher Baines schreef op za 25-06-2022 om 18:18 [+0100]:
>>              (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)
>
> I don't this would work if (current-error-port) has fd 1. Would
> move->fdes be appropriate here?  The following seems less fragile (*)
> to me (untested, also I didn't look at the context)
>
>   (move->fdes [child port] 0)
>   (move->fdes (dup [child port]) 1)
>   (if (file-port? (current-error-port))
>       (move->fdes (current-error-port) 2)
>       (move->fdes (open-file "/dev/null" O_WRONLY) 2))
>
>
> (*): move->fdes automatically moves ports out of the way.  Also, if one
> of the moves fails, then at least (current-output-port) etc will still
> have a correct fd so some error reporting should be possible

Maybe. I haven't actually tried this yet, but the docs seem to suggest
it would work.

Thanks,

Chris
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Mon, 27 Jun 2022 11:56:02 GMT) Full text and rfc822 format available.

Message #20 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 56218 <at> debbugs.gnu.org
Subject: Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Mon, 27 Jun 2022 12:39:55 +0100
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi Christopher,
>
> Christopher Baines <mail <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#56218; Package guix-patches. (Tue, 05 Jul 2022 15:14:02 GMT) Full text and rfc822 format available.

Message #23 received at 56218 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 56218 <at> debbugs.gnu.org
Subject: Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Tue, 05 Jul 2022 17:12:58 +0200
Hi,

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Hi Christopher,
>>
>> Christopher Baines <mail <at> 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’.




Reply sent to Christopher Baines <mail <at> cbaines.net>:
You have taken responsibility. (Fri, 08 Jul 2022 12:58:02 GMT) Full text and rfc822 format available.

Notification sent to Christopher Baines <mail <at> cbaines.net>:
bug acknowledged by developer. (Fri, 08 Jul 2022 12:58:02 GMT) Full text and rfc822 format available.

Message #28 received at 56218-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 56218-done <at> debbugs.gnu.org
Subject: Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of
 open-inferior #:error-port.
Date: Fri, 08 Jul 2022 13:54:24 +0100
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:

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

I've gone ahead and pushed a fix plus some tests as
a9fd06121240c78071a398dd1e0ddb47553f3809.

The tests probably aren't great, but I think the do cover the
#:error-port behaviour.
[signature.asc (application/pgp-signature, inline)]

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 06 Aug 2022 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 235 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.