Skip to content

Replace syscall.Unlink with os.Remove so that the directory(e.g. /run… #72

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Replace syscall.Unlink with os.Remove so that the directory(e.g. /run… #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sockets/unix_socket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func WithChmod(mask os.FileMode) SockOption {

// NewUnixSocketWithOpts creates a unix socket with the specified options
func NewUnixSocketWithOpts(path string, opts ...SockOption) (net.Listener, error) {
if err := syscall.Unlink(path); err != nil && !os.IsNotExist(err) {
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, so, this patch would not help getting into the situation (which is due to a race-condition between containers starting and the API being up), but would help recovering from that situation (by being able to remove the faulty directory instead of producing an error. Obviously, if there's containers still running that mount /var/run/docker.sock that would likely be problematic still 🤔

Change itself seems fine to me; if path is a directory it would attempt to remove it, which should be fairly safe, as it would fail if the directory is not empty, so there should be no real risk of accidentally removing files that we don't want to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah @tiborvass thanks.

For creating the dir of /var/run/docker.sock, the root cause of this is that dockerd can still handle the API when it shutdown, at that time, dockerd create a container and share the host path /var/run/docker.sock, but it is not exist，so dockerd create it. I have a pr moby/moby#38241 abort this，PTAL again, thanks very much.

return nil, err
}
mask := syscall.Umask(0777)
Expand Down