Add full'ish support for network socket get & set options

[slfritchie]

Hi, all. This is a "what do you think?" PR for adding runtime support for network socket getsockopt(2) and setsockopt(2) system calls and all (?) of the magic number constants that are required to use those system calls.

• Add @pony_os_rcvbuf() and @pony_os_sndbuf(), in the likeness (I hope) of other existing runtime functions that call getsockopt(2) and setsockopt(2). These would be shorthand or convenience functions to use instead of the more general @pony_os_getsockopt() and pony_os_setsockopt().

• Add functions to the Pony runtime for setsockopt(2) and getsockopt(2).

Constants for the level and option_name arguments to these
syscalls (2nd and 3rd args, respectively) can be accessed via
functions in the new primitive class OSSockOpt.
* NOTE: The usual (?) hack of using apply() to return these constant
values isn't used here. Please let me know if apply() really
is the preferred way to implement this. It seemed better to add one
primitive with 200+ functions instead of 200+ primitives.

I'm not sure which commonly-used (or rarely-used!) constants for
these syscalls that I've omitted. There are about 149 level
constants and about 88 option_name constants that I scraped out
of /usr/include header files for Linux, MacOS, and FreeBSD.
* I don't have a Windows development environment available, but it'd
be nifty to include any missing Windows constants also.

• Also, make the fd field of the TCPConnection actor public so that
  it's accessible for use with setsockopt and getsockopt without
  worry of race conditions that a behavior would be vulnerable to.

[SeanTAllen]

why expose this?

[slfritchie]

It's bullet point 3 in the description, sorry it isn't clearer. Is there a better way to allow TCPConnectionNotify.connected() or TCPListenNotify.connected() or .listening() to access the socket's file descriptor?

[slfritchie]

Hm, the following CircleCI error doesn't happen on OS X ... I'll look into it.

**** FAILED: net/http/_HTTPConnection._new_conn
listening on: 127.0.0.1:36274
received service: [36274]
URL: http://localhost:36274
url.string()=http://localhost:36274/
Test timed out without completing
Action never completed: client handler apply called 1
Action never completed: client handler apply called 2
Action never completed: server writing reponse 1
Action never completed: server writing reponse 2

Update: I can't get make test-ci config=release to fail on an Ubuntu Xenial VM. However, it does fail using the Docker image that Circle-CI is using. I'll continue looking into that unit test failure.

HOWEVER, there's another problem: ./stdlib --sequential does not exit promptly after the **** FAILED: 1 test, listed below: message. It seems like the runtime isn't detecting quiescence. That is an artifact of this specific unit test failure, I hope?

[slfritchie]

Summary from this week's Pony Sync meeting:

• I'll undo the _fd public'ification and instead see what adding functions to TCPListenNotify and TCPConnectionNotify feels like.
• The current file descriptor-centric API has a lot of history behind it but also isn't a superb reason to do bad things.
• The only "better" way to get those C preprocessor constants out of the compiler toolchain is to embed a "fraction" of LLVM into the Pony compiler. @sylvanc didn't recommend it. ^_^

[slfritchie]

All the CI checks are green/positive, hooray.

I changed 4 of the C setsockopt functions and made purely Pony variations, then added a not-quite-noop proposal for deprecating them. If there's an API deprecation policy already in place, please point me to it and I'll adjust accordingly.

(I noticed while making those changes that there's a likely cut-and-paste error in pony_os_multicast_ttl() using the wrong constant. This PR fixes both the C and Pony version.)

Last Wednesday's comments during the Pony Sync were helpful. I think another round of review would probably be good. Comments?

[jemc]

I still believe that the fd should not be exposed in this way - it's not really in line with the capability security model.

As I mentioned in the sync call, I'd prefer to see the "set socket option" and "get socket option" methods be moved here to TCPConnection, so that the fd can remain hidden.

[slfritchie]

The problem I ran into is that addressof can only be used for arguments to an FFI call. Is there a way to permit a pointer to be passed through the TCPConnection object boundary? I couldn't find a way, but I'm still new at that game, so I may have overlooked something.

Many useful getsockopt(2) and setsockopt(2) calls involve integers at 4 bytes. Creating a kludge where those things are copied across TCPConnection object boundary is a hack that I know how to do, using a Reader and Writer to . For any more complicated ones, such as IP_ADD_MEMBERSHIP and IP_DROP_MEMBERSHIP at https://github.com/ponylang/ponyc/blob/master/src/libponyrt/lang/socket.c#L1232-L1242 I don't know how to do in pure Pony strictly inside of TCPConnection without also resorting to Reader and Writer.

[jemc]

To be clear about what I'm envisioning:

• OSSocket goes away
  • option_size_changed_error gets moved probably to OSSockOpt
  • the rest of the functions in OSSocket get moved to be methods of TCPConnection, with the fd taken from the field instead of passed as an argument.

I don't think the above plan would run into any issues with addressof?

[jemc]

To be clear, the capability security issue I have with exposing the fd is that it is immutable in theory (a val reference in the type system) that in practice lets you do mutation on the socket. This is problematic for trying to enforce capability restrictions through the type system.

[slfritchie]

I've cooked up an example at http://playground.ponylang.org/?gist=59fb7dfdf9afbaa7798ad9b41b439735 of the compiler error that I don't know how to work around:

0.21.3 [release]
compiled with: llvm 3.9.1 -- cc (Ubuntu 5.4.0-6ubuntu1~16.04.5) 5.4.0 20160609
Error:
main.pony:11:7: the addressof operator can only be used for FFI arguments
      addressof optval,
      ^
Error:
main.pony:12:7: the addressof operator can only be used for FFI arguments
      addressof optlen)
      ^

[jemc]

Okay, the part I was missing was the part where you were still expecting some use cases to require the FFI interface instead of using OSSocket.

I took another look at your playground link. I still don't think the addressof is necessary to pass to the pony method - the same can be emulated by taking it as a normal argument and returning it in the return value. Or in cases like the one you had with an I32 option, there's no reason to pass one in, so you can just return it.

I noticed that you were assuming getsockopt would be a behaviour, but I don't think it should be - it should be a method that requires synchronous access to the TCPConnection, which only TCPConnectionNotify has. So I changed this in your playground link too.

http://playground.ponylang.org/?gist=5456427114b53e02c3f25022ae2c0027

EDIT: fixed the link.

[jemc]

I'd need to cut-and-paste those functions into udp_socket.pony

If you like, you could keep the FFI-wrapping implementations in a private primitive, which both TCPConnection and UDPSocket would have thin wrapper methods for, where they just pass their _fd in as the first argument.

Larger ones are pretty common, for example, passing pointers to (larger than 32 bit) C structures, such as the C code needed for IP_ADD_MEMBERSHIP, mentioned earlier today.

These can be wrapped as well - they just need a wrapper method that accounts for having the right signature.

[slfritchie]

After a chat with @SeanTAllen where neither of us are 100% clear of what you mean by a wrapper method, I'm wondering if pestering you later this week via chat might be possible?

One way I could interpret wrapper methods for right/proper signatures would be one wrapper per data size returned by the kernel. One example would be Linux's getsockopt( tcp_work_socket, SOL_IP, TCP_INFO, ...) where the value stored in arg4's pointer is a struct tcp_info, https://github.com/torvalds/linux/blob/2a17178/include/uapi/linux/tcp.h#L168-L227 for its value in master branch of Linus's tree today. Is the kind of wrapper that you're referring handle each of the members of that structure?

[jemc]

Maybe we should chat about it during or after the sync call on Wednesday.

However, to illustrate my idea, I created a playground link that shows example method signatures I'd expect to see: https://playground.ponylang.org/?gist=691db79876e8dea099be6f4f509f4c73

Your "one wrapper per data size" assumption was close, but it should actually be "one wrapper per Pony type".

[slfritchie]

Hmm, that's about what I thought you were suggesting. This feels like a case of a 80-20 or 90-10 rule. Lots of options are a single word, and I'd initially created helpers for that case.

But the remaining cases, there are many dozens hiding in the magic constants that I added to pony_os_sockopt_option, and likely more for anyone experimenting with network protocols in a custom kernel/kernel module(*). Even the struct tcp_info case that we've been discussing in the last day could likely change when someone decides to add support for a new RFC and adds even more members to that structure.

(*) That person probably also would not mind (too much) putting the magic constants into their Pony code rather than discovering them in a platform-independent way via @pony_os_sockopt_option.

[slfritchie]

Boooo, the net/Broadcast appears to have a race condition. I'm investigating it now.

[slfritchie]

I've changed the API substantially, thanks to the great review by @sylvanc @SeanTAllen @jemc and @kulibali from yesterday's sync meeting. My apologies if I've left anyone out. I hope that CI finds only small problems in this go-around.

Items to look at for another round of review could/ought to include?

• What have I broken on Windows platforms that aren't tested by CI?
• The API that I've implemented now is a bit different than was was discussed yesterday. Most of the change was to make it easier to write the docstrings for the new functions. A lot of documentation was plain ugly in most places where an optional argument was present.
• What ought to be changed in the documentation?
• How horrible is the new C function api_deprecated()? Should it do anything more than be a no-op placeholder?

[Praetonus]

The endianness stuff isn't exactly right. The current checks in target_is_big/littleendian return the endianness of the host platform, which isn't correct in a cross-compilation context. The functions should mirror the other target_is_* functions and use the LLVM API for target triples. The function to use here is llvm::Triple::isLittleEndian().

In addition, endianness should be covered in buildflagset.c in the same way OSes & co are covered, with mutually exclusive groups. The Platform primitive in the standard library should also get new functions for endianness, with the new associated compiler intrinsics that should be defined in genprim.c.

[slfritchie]

Hi, @Praetonus, thanks for pointing me at the right places for adding new bits to the compiler and the Platform primitive. The three commits up to b2d7443 try to address your review change requests, though I'm not at all sure that I've done the right thing. There was a lot of "copy what's done with other flags tables then change the variable names". Please holler if I've missed or abused anything.

[mfelsche]

should all those methods setting sock opts be fun ref instead of fun box as they kind of mutate the connection (although it is an actor)?

[slfritchie]

Oops, yes. And that was also mentioned back in a Sync meeting, sorry, I'll fix it up shortly.

[Praetonus]

The compiler changes to endianness look good to me.

The requested changes have been implemented.

[mfelsche]

A usage example would be nice.

Also the information that the array should be pre allocated the expected result size is missing.

[slfritchie]

I've added the examples requested by @mfelsche. AFAIK, this PR is mergeable. If someone has a different opinion, please shout.

However, @dipinhora made an good suggestion to be last night to change the API of getsockopt. I'm going to try it out today. If it's quick, I'll add it to this PR. It will make the example code I added less verbose by making the use of Reader less cumbersome.

[jemc]

Yep, should be mergeable.

[slfritchie]

I'd forgotten that I was going to remove the api_deprecated() C function that I added in the PR. I'll remove it now and use the new push as an excuse to tickle the Appveyor CI into retrying whatever timeout happened at https://ci.appveyor.com/project/ponylang/ponyc/build/ponyc-0.21.3-4082.10c6d82/job/1xyurgbwhjx9o6at.