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

Added basic AF_UNIX support to asyncnet. #9877

Closed
wants to merge 1 commit into from

Conversation

zevv
Copy link
Contributor

@zevv zevv commented Dec 6, 2018

Unfortunately this required some code duplication because the doConnect() from asynccommon.nim only works with addrInfo which does not make sense for AF_UNIX.

Also needed to export makeUnixAddr() from net.nim.

@Araq Araq requested a review from dom96 December 6, 2018 09:48
@Araq
Copy link
Member

Araq commented Dec 6, 2018

Docgen fails with

nativesockets.nim(487, 37) Error: undeclared identifier: 'nativeAfUnix'

Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Other than those small things I think it looks good.

lib/pure/net.nim Outdated
@@ -948,7 +948,7 @@ proc setSockOpt*(socket: Socket, opt: SOBool, value: bool, level = SOL_SOCKET) {
setSockOptInt(socket.fd, cint(level), toCInt(opt), valuei)

when defined(posix) and not defined(nimdoc):
proc makeUnixAddr(path: string): Sockaddr_un =
proc makeUnixAddr*(path: string): Sockaddr_un =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be moved to nativesockets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

raise newException(IOError, "Unknown socket family in getAddrString")
when defined(posix):
if sockAddr.sa_family.cint == nativeAfUnix:
return "unix"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but I would give it its own elif branch above and check for posix there. That way you can give a big more meaningful error like "Unix socket family not supported on Windows".

Copy link
Contributor Author

@zevv zevv Dec 9, 2018

Choose a reason for hiding this comment

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

But this requires nativeAfUnix to be defined also for non posix, but what value should it get here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that's fine. Another thing regarding this proc, shouldn't it return the Unix socket file path?

Copy link
Contributor Author

@zevv zevv Dec 9, 2018

Choose a reason for hiding this comment

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

Not sure what the semantics of this function are, is it supposed to be the Nim implementation of getsockname()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe so. Based on the other code in this function it should return the string representation of a SockAddr, "unix" isn't specific enough.

Copy link
Contributor Author

@zevv zevv Dec 9, 2018

Choose a reason for hiding this comment

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

Funny, sun_path seems not to be filled in by the kernel after accepting a unix socket...

I'm not sure this can be done here. I need the socket itself to get the peer address with getsockname(), but the sockAddr passed to getAddrString simply does not contain the info after doing the accept().

@@ -622,6 +622,45 @@ proc bindAddr*(socket: AsyncSocket, port = Port(0), address = "") {.
raiseOSError(osLastError())
freeAddrInfo(aiList)

when defined(posix) and not defined(nimdoc):
Copy link
Contributor

Choose a reason for hiding this comment

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

These procedures shouldn't be behind a not defined(nimdoc). It would be nice to also give them docs and specify they are only available for POSIX.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, you'll need to do something like:

when defined(posix):
  # Implementation
elif defined(nimdoc):
  proc connectUnix*(socket: AsyncSocket, path: string): Future[void] =
    ## ...
    discard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

result = retFuture

proc cb(fd: AsyncFD): bool =
let ret = SocketHandle(fd).getSockOptInt(cint(SOL_SOCKET), cint(SO_ERROR))
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get this code from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stolen from doConnect() in asynccommon

some code duplication because the doConnect() from asynccommon.nim only
works with addrInfo which does not make sense for AF_UNIX.
Also needed to export makeUnixAddr() from net.nim.
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.

3 participants