Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid keeping @docker_cli_[UUID] files #4862

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Conversation

catap
Copy link
Contributor

@catap catap commented Feb 8, 2024

Seems that OpenBSD behaves like darwin and requires to unlink all socket, after it was used.

Tested on OpenBSD 7.4


func listen(socketname string) (*net.UnixListener, error) {
return net.ListenUnix("unix", &net.UnixAddr{
Name: "@" + socketname,
Copy link
Member

Choose a reason for hiding this comment

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

Curious; was there a reason to differentiate from the Darwin implementation here? Looks like the difference here with the Darwin one is that that one uses the system's temp-dir as an extra precaution from these ending up in the working-directory (which could also be the build-context);

Name: filepath.Join(os.TempDir(), socketname),

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 good point. Mainly becuase it has naming darwin and nondarwin :)

But I like this way more, let me try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works like a charm

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's great!

I'm not at my computer right now; are the openBSD and Darwin implementations now exactly the same?

If they are, perhaps we should use the same file for both then. I guess the file suffix should be renamed to something else the (_nolinux? _other?) good suggestions for that welcome 😅

cc @laurazard

Copy link
Contributor Author

@catap catap Feb 12, 2024

Choose a reason for hiding this comment

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

And better to rename nodarwin as well.

But I really have not idea regarding naming.

no Linux seems wrong because at least FreeBSD exists which behavior is unknown for me.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for missing this one; the naming is pretty obvious IMO as FreeBSD also does not have abstract sockets (they are a Linux-ism); we just go with abstract and noabstract 😄

Seems that OpenBSD behaves like darwin and requires to unlink all
socket, after it was used.

Tested on OpenBSD 7.4

Signed-off-by: Kirill A. Korinsky <kirill@korins.ky>
@codecov-commenter
Copy link

Codecov Report

Merging #4862 (2c21424) into master (324309b) will increase coverage by 0.02%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4862      +/-   ##
==========================================
+ Coverage   61.29%   61.32%   +0.02%     
==========================================
  Files         287      287              
  Lines       20041    20058      +17     
==========================================
+ Hits        12285    12300      +15     
- Misses       6865     6866       +1     
- Partials      891      892       +1     

Copy link
Collaborator

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

Good catch! LGTM

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Naming is hard, and I don't have a good suggestion for #4862 (comment)

It's just a couple of lines of duplicated code, and we can easily fix in future if we come up with a name. I guess the only risk is for them to diverge, but let's keep that for a follow up 😅

LGTM

@thaJeztah thaJeztah merged commit d7f1958 into docker:master Feb 12, 2024
77 checks passed
@thaJeztah thaJeztah added this to the 26.0.0 milestone Feb 12, 2024
@catap catap deleted the openbsd branch February 12, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants