Return nil when openFifo returns nil by corhere · Pull Request #47 · containerd/fifo (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation5 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
The return statement in the OpenFifo wrapper function implicitly boxes the *fifo pointer into an io.ReadWriteCloser interface value, even when the value is (*fifo)(nil). It is the same gotcha described in https://go.dev/doc/faq#nil_error and causes the same sorts of confusing situations. Modify the OpenFifo wrapper function to return a nil interface value when openFifo returns a nil pointer value.
Signed-off-by: Cory Snider csnider@mirantis.com
| @@ -42,15 +42,16 @@ func TestFifoCancel(t *testing.T) { |
|---|
| leakCheckWg = nil |
| }() |
| _, err = OpenFifo(context.Background(), filepath.Join(tmpdir, "f0"), syscall.O_RDONLY|syscall.O_NONBLOCK, 0600) |
| f, err := OpenFifo(context.Background(), filepath.Join(tmpdir, "f0"), syscall.O_RDONLY|syscall.O_NONBLOCK, 0600) |
| assert.Exactly(t, nil, f) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annoyingly, assert.Nil considers non-nil interface values which contain nil concrete values to be nil values despite those values being != nil.
Oh! Saw the PR title and had to think of #32, but I see you linked that above, nice 👍
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
| assert.NotNil(t, err) |
|---|
| assert.NoError(t, checkWgDone(leakCheckWg)) |
| ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) |
| defer cancel() |
| f, err := OpenFifo(ctx, filepath.Join(tmpdir, "f0"), syscall.O_RDONLY|syscall.O_CREAT |
| f, err = OpenFifo(ctx, filepath.Join(tmpdir, "f0"), syscall.O_RDONLY|syscall.O_CREAT |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Not a blocker (at all), but if you want to reduce code-churn, perhaps reformat the octal values to the new format (0o600) while updating these lines
This was referenced
Feb 21, 2023