[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

Debbugs page

Christopher Baines wrote 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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 3 years ago

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

To respond to this issue using the mumi CLI, first switch to it

mumi current 56218

Then, you may apply the latest patchset in this issue (with sign off)

mumi am -- -s

Or, compose a reply to this issue

mumi compose

Or, send patches to this issue

mumi send-email *.patch

You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags

mumi command -t +confirmed -t +easy

Or, remove the moreinfo tag and set the help tag

mumi command -t -moreinfo -t +help

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