{"action":"created","issue":{"url":"https://api.github.com/repos/mono/mono/issues/1977","labels_url":"https://api.github.com/repos/mono/mono/issues/1977/labels{/name}","comments_url":"https://api.github.com/repos/mono/mono/issues/1977/comments","events_url":"https://api.github.com/repos/mono/mono/issues/1977/events","html_url":"https://github.com/mono/mono/pull/1977","id":101050803,"number":1977,"title":"Add socket-related functions and data structures to Mono.Posix","user":{"login":"steffen-kiess","id":947515,"avatar_url":"https://avatars.githubusercontent.com/u/947515?v=3","gravatar_id":"","url":"https://api.github.com/users/steffen-kiess","html_url":"https://github.com/steffen-kiess","followers_url":"https://api.github.com/users/steffen-kiess/followers","following_url":"https://api.github.com/users/steffen-kiess/following{/other_user}","gists_url":"https://api.github.com/users/steffen-kiess/gists{/gist_id}","starred_url":"https://api.github.com/users/steffen-kiess/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/steffen-kiess/subscriptions","organizations_url":"https://api.github.com/users/steffen-kiess/orgs","repos_url":"https://api.github.com/users/steffen-kiess/repos","events_url":"https://api.github.com/users/steffen-kiess/events{/privacy}","received_events_url":"https://api.github.com/users/steffen-kiess/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-08-14T16:44:24Z","updated_at":"2015-08-19T00:21:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mono/mono/pulls/1977","html_url":"https://github.com/mono/mono/pull/1977","diff_url":"https://github.com/mono/mono/pull/1977.diff","patch_url":"https://github.com/mono/mono/pull/1977.patch"},"body":"This commit adds socket-related functions and data structures to Mono.Posix.\r\n\r\nThis is evertything from &lt;sys/socket.h&gt; and the structures sockaddr_in, sockaddr_in6 and sockaddr_un.\r\n\r\nA few comments regarding the implementation:\r\n- sockaddr_* is passed into and out of the functions as a byte[] (+ a long for the length of the data). This is because the actual type of the data is unknown, as it depends on the address family. The Sockaddr* structures can be used to create or inspect such a byte[].\r\n- control messages are also treated as an opaque byte sequence, which can be created or inspected with the CMSG_* macros. The unit tests contain an example for sending file descriptors over a socket using SCM_RIGHTS\r\n- The enums (e.g. SockType) have been named in a way to avoid conflicts when importing the namespaces System.Net or System.Net.Sockets and Mono.Unix.Native. (Several files in the mono repository do this, probably also projects outside the mono repository do the same thing.)\r\n\r\nIf you have any questions / comments or want me to change something please tell me so.\r\n"},"comment":{"url":"https://api.github.com/repos/mono/mono/issues/comments/132401363","html_url":"https://github.com/mono/mono/pull/1977#issuecomment-132401363","issue_url":"https://api.github.com/repos/mono/mono/issues/1977","id":132401363,"user":{"login":"jonpryor","id":155958,"avatar_url":"https://avatars.githubusercontent.com/u/155958?v=3","gravatar_id":"","url":"https://api.github.com/users/jonpryor","html_url":"https://github.com/jonpryor","followers_url":"https://api.github.com/users/jonpryor/followers","following_url":"https://api.github.com/users/jonpryor/following{/other_user}","gists_url":"https://api.github.com/users/jonpryor/gists{/gist_id}","starred_url":"https://api.github.com/users/jonpryor/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonpryor/subscriptions","organizations_url":"https://api.github.com/users/jonpryor/orgs","repos_url":"https://api.github.com/users/jonpryor/repos","events_url":"https://api.github.com/users/jonpryor/events{/privacy}","received_events_url":"https://api.github.com/users/jonpryor/received_events","type":"User","site_admin":false},"created_at":"2015-08-19T00:21:21Z","updated_at":"2015-08-19T00:21:21Z","body":"Overall, a few comments.\r\n\r\nI have *not* finished reviewing the PR. This is a very large PR, making review more difficult than is desirable. What would be handy is if the PR were split up between \"trivial\" bindings such as **socket**(2) and...everything else, with a PR per \"area\", e.g. a PR for `Msghdr` support, a PR for `Sockaddr*`, etc.\r\n\r\nMaybe that wouldn't actually help, but I think it might...\r\n\r\nMore importantly is the abstract issue of \"API design.\" I *do not* want to expose APIs which require that developers know *more* than what they'd need to know in *C*.\r\n\r\nRephrased: anything that requires `byte[]` parameters that \"actually\" contain C struct types is largely a non-starter. It may be difficult, but there *has* to be a better way.\r\n\r\nFor example, **bind**(2):\r\n\r\n    /* C */\r\n    int bind (int socket, const struct sockaddr *address, socklen_t address_len);\r\n\r\n[Example use](http://www.tutorialspoint.com/unix_sockets/socket_server_example.htm):\r\n\r\n    struct sockaddr_in serv_addr = { 0, };\r\n    serv_addr.sin_family       = AF_INET;\r\n    serv_addr.sin_addr.s_addr  = INADDR_ANY;\r\n    serv_addr.sin_port         = htons(5001);\r\n    bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr));\r\n\r\nThis PR provides three overloads for this:\r\n\r\n    public static unsafe int bind (int socket, byte *addr, long addr_len);\r\n    public static unsafe int bind (int socket, byte[] addr, long addr_len);\r\n    public static unsafe int bind (int socket, ISocket address);\r\n\r\nOf these, I don't like the first two: they require too much knowledge, they diverge \"too much\" from POSIX to use. The third one is okay, as the IDE will provide code completion, suggesting use of `SockaddrIn`/etc.\r\n\r\nThat said, I don't like ISocket, and think that we could do \"one better\".\r\n\r\nWhat `struct sockaddr` is *trying* to do is, more or less, be \"object oriented.\" There's a \"base class\" of `struct sockaddr`, and multiple \"subclasses\" in `struct sockaddr_in`, `struct sockaddr_un`, etc.\r\n\r\nWe have a proper object oriented language: C#. What's more, we can use `[StructLayout]` to marshal *classes*, [including inheritance](https://gist.github.com/jonpryor/f121ba42e12742e172b4).\r\n\r\nWhich means byte pointers/arrays and interfaces and array allocations aren't necessary; the default marshaler should suffice:\r\n\r\n    [StructLayout(LayoutKind.Sequential)]\r\n    public class Sockaddr {\r\n        // http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_socket.h.html\r\n        [sa_family_t] public AddrFamily sa_family;\r\n    }\r\n\r\n    [StructLayout(LayoutKind.Sequential)]\r\n    public class SockaddrStorage : Sockaddr {\r\n    }\r\n    public class SockaddrIn : Sockaddr {\r\n    }\r\n    // ...\r\n\r\nThis would allow a saner binding:\r\n\r\n    public static unsafe int bind (int socket, Sockaddr address);\r\n    ...\r\n    var serv_addr = new SockaddrIn {\r\n        sin_family = AddrFamily.AF_INET,\r\n        sin_addr = {\r\n            s_addr = // ???\r\n        }\r\n        sin_port = 5001, // yeah, wrong...\r\n    };\r\n    bind(sockfd, serv_addr);\r\n\r\nSuch a public API may require type checks and/or/extra code, but it's *miles* better than needing to deal with `byte[]`.\r\n\r\n`ISockaddr` could be a useful compromise, but using it requires the `ToNativeArray()` method, which always requires a memory allocation, which I *think* could be avoided.\r\n\r\nThe same \"`byte[]` overloads\" also apply to **connect**(2), **accept**(2), **getsockname**(2), etc.\r\n\r\nFor example, look at your test for `getsockname()`:\r\n\r\n    var actualAddress = new byte[Sockaddr.SizeofSockaddrStorage];\r\n    long len          = actualAddress.Length;\r\n    if (Syscall.getsockname (so1, actualAddress, ref len) < 0)\r\n    \tUnixMarshal.ThrowExceptionForLastError ();\r\n    var port = SockaddrIn.FromNativeArray (actualAddress).sin_port;\r\n\r\nHow are developers supposed to \"know\" that `actualAddress` is *actually* a `SockaddrIn`? How are they supposed to know about `Sockaddr.SizeofSockaddrStorage`? (Why is it \"SizeofSockaddrStorage\" for a \"SockaddrIn\" instance?)\r\n\r\nThis API is developer hostile; there's know way of using the API without knowing the ins-and-outs, and there's no way of knowing that without documentation or reading the implementation+tests.\r\n\r\nWhat this should instead aim for is:\r\n\r\n    var address = new SockaddrIn ();\r\n    Syscall.getsockname (so1, address);\r\n    var port = address.sin_port;\r\n\r\nNormal code completion for `Syscall.getsockname()` will provide all `Sockaddr` subclasses -- which will requiring knowing which is actually the best type -- `SockaddrIn` or `SockaddrStorage`? -- but at least the names are \"close enough\" to POSIX that reading existing C code should be sufficient to muddle through. Byte arrays? No. Please, no."}}