| Message ID | 20220625171847.29104-1-mail@cbaines.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#56218] guix: inferior: Fix the behaviour of open-inferior #:error-port. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
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.
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.
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. > +++ 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’.
Maxime Devos <maximedevos@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
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. >> +++ 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
Hi, Christopher Baines <mail@cbaines.net> skribis: > 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. >>> + (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’.
Ludovic Courtès <ludo@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.
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))))