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

DoneSubmitted by Christopher Baines.
Details
3 participants
  • Ludovic Courtès
  • Christopher Baines
  • Maxime Devos
Owner
unassigned
Severity
normal
C
C
Christopher Baines wrote on 25 Jun 19:18 +0200
(address . guix-patches@gnu.org)
20220625171847.29104-1-mail@cbaines.net
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(-)

Toggle diff (24 lines)
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
M
M
Maxime Devos wrote on 25 Jun 19:50 +0200
e10b2fcd29cf540e80453a19eb8973f636153d91.camel@telenet.be
Christopher Baines schreef op za 25-06-2022 om 18:18 [+0100]:
Toggle quote (15 lines)
>              (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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYrdK1BccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7sm6AP0a9A7nub6rSATS8jNEbox8gkMz
5fHUcI4z8wkxiR4KIAEAsi/pr7LCshHQxvVJLWPEOBG9ax7CHUcC5Mz09Hn4/w4=
=fceQ
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 25 Jun 22:59 +0200
6d23e4d641e1273fb01bdc35a24050b6236a4d25.camel@telenet.be
Maxime Devos schreef op za 25-06-2022 om 19:50 [+0200]:
Toggle quote (2 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYrd3ORccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7sUeAQD8fSsuoJcDeTxBJimIWuJfXyCH
U2w4uuYLe8KfZ++FlwD/RKoOx0lWYRmil6MLA6WTFBp5WWabB2s9Q9rfvsVIdA8=
=/9xV
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 26 Jun 17:46 +0200
Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of open-inferior #:error-port.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 56218@debbugs.gnu.org)
87v8snqvf2.fsf@gnu.org
Hi Christopher,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (3 lines)
> 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.

Toggle quote (17 lines)
> +++ 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’.
C
C
Christopher Baines wrote on 27 Jun 13:37 +0200
Re: [bug#56218] [PATCH] guix: inferior: Fix the behaviour of open-inferior #:error-port.
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56218@debbugs.gnu.org)
87k092l4hn.fsf@cbaines.net
Maxime Devos <maximedevos@telenet.be> writes:
Toggle quote (31 lines)
> [[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
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmK5lwRfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XdHAQ/9FOZ+tTSCY7vOpcJurYeKLmc0tzEGy7+L
HJpmcB4j5RahbNveHs0arbd+/54lo3hHfPgBhQNF5wHV7o6Owt9Zwlzm66e8C93H
YEDx/w1wkzmfw+txgWBswgX6imCoJC1YEM+OVWv/khoe6kvfRuMfwQ2QHJwIp/nm
YFnmoEjZOMclLO1RaAe5+iF0Lok7dfpAAZDF8a1rJW6fcscHS4NhB3A9RJMVg8PH
b/WdxgJvG4di1fpLH937gIEuzGIb85TAryZyKnIn24HSYPKyzRq7D3U87G/WpvrH
x6hrNismkfuDz5l94BrBQspUQIrfgGhYvFvjQbQT4u+luGwNkm9ISg2LuU0dsevd
oT/5rbPD66caXeNa3SE2R6C5UkIjlA6V5Kiiyk9cKCAlQewPxuGW8P0jIZc7gvip
dtSYPJmOSe/PT5FVT7wIdb91iG001Oxs6tnlv6oCTO9hYslemTEhNTOmqdJ8FdE0
QSjesG10Jx0BTid1Mh9oCN+q3Jh0w2jUu7DbJnGAlj1pftsCkvATDA9hK80NOQV6
UA+MXdiTw7JYMH449H0Wvt2rVNc5xCk5gx4NfZpZzY4EeI4+4SW/fnpplNeCjhve
9eHUsAD1Xp2u6WRH1jQwSL0dOd/vucCfFAXJf9wk/QtsPq/LuLEry5+vI/fZeOjW
1AC7LSvZmhw=
=BHgT
-----END PGP SIGNATURE-----

C
C
Christopher Baines wrote on 27 Jun 13:39 +0200
Re: bug#56218: [PATCH] guix: inferior: Fix the behaviour of open-inferior #:error-port.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 56218@debbugs.gnu.org)
87fsjql3rw.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (12 lines)
> 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.

Toggle quote (23 lines)
>> +++ 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))

Toggle quote (3 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmK5mqNfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XchPQ//YAAcZAUwEHAHb8NWWU91G1cr1xgacXfP
mLwyCr3kREI8yd3JBfuj84bR2NRMz5ddvxC/pveR7hh/91hEvpR7n0RTkFXO9P7i
B15qLNJ87QDraX4AS0qMvhY7adbNRD7U7fzK4jXY7uqUln4SEdd9IppW+EivIipy
3YQVbSNl6YCWT2Ap+LPIowXl5r3WHhVGSQKApqIC2IqHmmgW/4VTiO3GClRqlIBl
nrG4buaQMCj/EOaki03bckqh6KPSeoGwCyRDCcqPjrfBDg3nOhOHZSsBt0Mooktq
+mc2Icy6Ibmibu9c2hckwioc17v2TvuHUcOWYD6w9Mbhmmx4fc9EbxXHMxx7wEnK
MeWaWwYQwGvYT1KRMbEa8XbFJjBf0Ugyogu1c2WJohYds2n8yYqmbr10dJhCw/m7
rRcB0CI7MkeQgk2lVzMKFb4+6GmwY4PLCTOju7UcszsWwE2ubZ9BGLScUSstBZl1
78aM5giS3M7TrwIJj81rMeIvVPCdiFuF0fNfwdJWcS1pZQLyE0akfyvqqtzyKJTM
sf1IWetVbErUcBfzGZEdkqmBOU6aSSr6esed+CuQDxTDCH5CxO4Q984p/DT83jVA
H7dsOViglcMuE9B4h3Gl57+DAp/iRFZusQrfsVJtX75HdHjioP7nV3hnhrRq4BU0
ijd1psGfW3M=
=x3+O
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 5 Jul 17:12 +0200
(name . Christopher Baines)(address . mail@cbaines.net)(address . 56218@debbugs.gnu.org)
87o7y34mpx.fsf_-_@gnu.org
Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (20 lines)
> 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.

Toggle quote (24 lines)
>>> + (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 :-)).

Toggle quote (5 lines)
>> 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’.
C
C
Christopher Baines wrote on 8 Jul 14:54 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 56218-done@debbugs.gnu.org)
87ilo792zl.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (36 lines)
>>>> + (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.
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmLIKZ5fFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9Xe74xAAiqmV9xooPtT1HHm8P/fj61XH8xO7pLOR
EdSgsC5IVvBefxzPg6okh4NOGVq84/QBZO4p7eKlckp/5Oh5EjXOe/1/JLNJytd9
mqRnJv6jLtavQNQiQvdMkaSjt5ciWOpnbhjyKt9Gk5tB2Isb6JjliZxPK9I3Zh10
SeCvvAEwgGqvYR2e+176nHOKyYVX6iLaUd2tZV+MUWl1MX/05F5DSTNVF3t3fCZb
SO/WjjCdFyDqL7l4is01ei//EVwml/Seb3TX/3Y24vHq0+Lyg0iXkhPrcJNKG37B
ZitCflZ9T22HnZg6gEHpKnIQUzzACvaFJAFcc93ZeOM1fOFIlfVpcbZjII7we1Lt
6WYNXOq0ZOnQ7HS3+e8/H/rgRiwQQ0VLpyBPFi0+oWu+XrwBIXQ0bbt3xHLabnJV
iNdoBiYvk6/nsQD6bPmneIjz1QkrD1She/GnKdFQaj7yvw4GsvQnC0h46IYrBSKM
VWpQvtuToxbWDSmjKn42t1l1RwwnsxswSFvcCNaa7bguvqj+nIPDNo/xTTyP0E7c
s460cJuhUBirW1vtDUVu4rv0+c4Yq/UCd6SVlUiDebpwIe4NhQaNVmm+eEn7YQTa
86zeHzEapQzmMpRvl0ZtGUMK67OcE5tTm8rs4bJrrpvmXxTmQ8zSL2bcK8P0YOI3
J5ILVTo0cBE=
=tEX8
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 56218@debbugs.gnu.org