GNU bug report logs -
#56218
[PATCH] guix: inferior: Fix the behaviour of open-inferior #:error-port.
Previous Next
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.
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):
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):
[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):
[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):
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):
[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):
[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):
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):
[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.