util_linux: add connection close in setupIO #4780
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,11 +123,13 @@ func setupIO(process *libcontainer.Process, container *libcontainer.Container, c | |
| } | ||
| uc, ok := conn.(*net.UnixConn) | ||
| if !ok { | ||
| conn.Close() | ||
|
There was a problem hiding this comment. I don't understand, if the result is not ok can the connection still be open. Are you sure? |
||
| return nil, errors.New("casting to UnixConn failed") | ||
| } | ||
| t.postStart = append(t.postStart, uc) | ||
| socket, err := uc.File() | ||
| if err != nil { | ||
| conn.Close() | ||
|
There was a problem hiding this comment. Let's avoid having to do this on every exit path. Please create a defer after the connection is established, that is an inline function and does a conn.Close only if we are returning an error. There was a problem hiding this comment. And if we put defer, won't we close the connection in any case (success and error) when exiting the function? Or do you mean that a simple defer is not needed? There was a problem hiding this comment. We can do a defer that checks if we are returning with error. See for example here: Lines 123 to 127 in c3d127f You defer a function, and you have a named return. Then, in that function you check if the error we are returning is not nil, then you do the close. I think we can use the same here, can't we? There was a problem hiding this comment. I do it like that but have failed in CI. What's wrong? |
||
| return nil, err | ||
| } | ||
| t.postStart = append(t.postStart, socket) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution.
In practice, omitting these close operations doesn't cause any harm, since runc will exit anyway if an error occurs during the setupIO call.
However, from a code style and best practices perspective, I think we should always close the socket connection once an error is encountered after the connection has been established.
Considering both aspects, LGTM.