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

std.os: add support for socketpair #18561

Closed
wants to merge 4 commits into from

Conversation

notcancername
Copy link
Contributor

@notcancername notcancername commented Jan 14, 2024

fixes #18417
cc @nektro

with the AF_UNIX implementation, the file the socket is bound to is not cleaned up, it might be worth exposing this eventually or abstracting socketpair properly in std.net (related: #6600), comments are appreciated

(also: fix an error where recvfrom would use std.c instead of std.os.windows even when libc is not linked)

Copy link
Contributor

@nektro nektro left a comment

Choose a reason for hiding this comment

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

some thoughts in no particular order

  • nice!
  • if windows doesn't have this behavior I don't think std.os should pretend it does imo.
  • additionally it should accept i32 or c_int and not bitCast
  • E.OPNOTSUPP, E.PROTONOSUPPORT shouldn't be in the same case
  • neither should E.NOBUFS, E.NOMEM
  • there shouldn't be util functions, it should all be in the if statement of socketpair
  • common style for std.os is if (windows) { _win_ } _posix_, this is in the wrong order and shouldn't use an else

@notcancername
Copy link
Contributor Author

notcancername commented Jan 15, 2024

  • if windows doesn't have this behavior I don't think std.os should pretend it does imo.

Seems like std.net is the preferred layer for this?

  • E.OPNOTSUPP, E.PROTONOSUPPORT shouldn't be in the same case
  • neither should E.NOBUFS, E.NOMEM

Which errors would be more appropriate?

there shouldn't be util functions, it should all be in the if statement of socketpair

They're separate functions because AF_UNIX may not be available at runtime:

        error.AddressFamilyNotSupported => return socketpairEmulatedAfInet(),

additionally it should accept i32 or c_int and not bitCast

std.c.socketpair uses c_uint, but std.os.linux.socketpair uses i32. Some amount of casting was unavoidable. I've changed std.os.linux.socketpair to fit std.os.socket precedent.

@nektro
Copy link
Contributor

nektro commented Jan 15, 2024

@nektro
Copy link
Contributor

nektro commented Jan 15, 2024

        .OPNOTSUPP => return error.OperationNotSupported,
        .PROTONOSUPPORT => return error.ProtocolNotSupported,
        .NOBUFS => return error.SystemResources,
        .NOMEM => return error.SystemResources,

@notcancername notcancername force-pushed the socketpair branch 2 times, most recently from e8778d1 to c7c3fb3 Compare January 15, 2024 01:43
@notcancername
Copy link
Contributor Author

done. why are we using separate switch prongs for the same error?

@nektro
Copy link
Contributor

nektro commented Jan 15, 2024

no particular reason afaik. just the style that core team/prs has kept up so far in this case to keep things vertical

@notcancername
Copy link
Contributor Author

these test failures are looking pretty mysterious:

 src/Sema.zig:32601:92: error: no field or member function named 'toValue' in 'InternPool.Index'
src/InternPool.zig:2010:19: note: enum declared here
child process failed
Aborted
Error: Process completed with exit code 134.

I don't see how my changes could possibly have caused this

@nektro
Copy link
Contributor

nektro commented Jan 15, 2024

looks like master branch is failing too

@@ -452,3 +452,5 @@ pub extern "kernel32" fn RegOpenKeyExW(
) callconv(WINAPI) LSTATUS;

pub extern "kernel32" fn GetPhysicallyInstalledSystemMemory(TotalMemoryInKilobytes: *ULONGLONG) BOOL;

pub extern "kernel32" fn GetTempPathA(nBufferLength: DWORD, lpBuffer: LPSTR) callconv(WINAPI) DWORD;
Copy link
Member

Choose a reason for hiding this comment

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

no

@andrewrk andrewrk closed this Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

std.os is missing socketpair
3 participants