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

Done

Details

3 participants

Ludovic Courtès
Christopher Baines
Maxime Devos

Owner: unassigned

Submitted by: Christopher Baines

Severity: normal

Christopher Baines wrote on 25 Jun 2022 19:18

Recipients:(address . guix-patches@gnu.org)

Message-ID: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

Maxime Devos wrote on 25 Jun 2022 19:50

Recipients:

Message-ID: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-----

Maxime Devos wrote on 25 Jun 2022 22:59

Recipients:

Message-ID: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-----

Ludovic Courtès wrote on 26 Jun 2022 17:46

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

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 56218@debbugs.gnu.org)

Message-ID: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’.

Christopher Baines wrote on 27 Jun 2022 13:37

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

Recipients:(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56218@debbugs.gnu.org)

Message-ID: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

Christopher Baines wrote on 27 Jun 2022 13:39

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

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 56218@debbugs.gnu.org)

Message-ID: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

Ludovic Courtès wrote on 5 Jul 2022 17:12

Recipients:(name . Christopher Baines)(address . mail@cbaines.net)(address . 56218@debbugs.gnu.org)

Message-ID: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’.

Christopher Baines wrote on 8 Jul 2022 14:54

Recipients:(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 56218-done@debbugs.gnu.org)

Message-ID: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.

Closed

Your comment

This issue is archived.

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

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date