[Mono.Posix] Add support for sending and receiving socket control messages #2097

Merged
merged 1 commit into from Jan 25, 2016

Conversation

Projects
None yet
5 participants
@steffen-kiess
Contributor

steffen-kiess commented Sep 29, 2015

Add wrappers for struct cmsghdr, the macros for manipulating control messages (CMSG_*) and recvmsg() and sendmsg().

This commit is the third and last part of #1977

This pull request depends on #2006

@monojenkins

This comment has been minimized.

Show comment
Hide comment
@monojenkins

monojenkins Sep 29, 2015

Contributor

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

Contributor

monojenkins commented Sep 29, 2015

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

+ Marshal.Copy (buffer, 0, (IntPtr) ptr, 4);
+ }
+
+ public unsafe void CopyTo (byte[] destination, int startIndex)

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

You have a struct which is mutable (always dubious...), and a way to set the bytes (via the indexer), and a way to copy the bytes out...but no way to copy them in, other than the constructor.

I'm wondering...lots of things, but should there be a Copy(byte[], int) overload for copying int InAddr?

Alternatively, should this instance instead be immutable, and the indexer setter should be removed?

@jonpryor

jonpryor Oct 7, 2015

Member

You have a struct which is mutable (always dubious...), and a way to set the bytes (via the indexer), and a way to copy the bytes out...but no way to copy them in, other than the constructor.

I'm wondering...lots of things, but should there be a Copy(byte[], int) overload for copying int InAddr?

Alternatively, should this instance instead be immutable, and the indexer setter should be removed?

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

You have a struct which is mutable (always dubious...), and a way to set the bytes (via the indexer), and a way to copy the bytes out...but no way to copy them in, other than the constructor.

I'm wondering...lots of things, but should there be a Copy(byte[], int) overload for copying int InAddr?

Alternatively, should this instance instead be immutable, and the indexer setter should be removed?

Most (or all?) of the other structs in Mono.Unix.Native are also mutable, so I'll add a CopyFrom(byte[], int) method.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

You have a struct which is mutable (always dubious...), and a way to set the bytes (via the indexer), and a way to copy the bytes out...but no way to copy them in, other than the constructor.

I'm wondering...lots of things, but should there be a Copy(byte[], int) overload for copying int InAddr?

Alternatively, should this instance instead be immutable, and the indexer setter should be removed?

Most (or all?) of the other structs in Mono.Unix.Native are also mutable, so I'll add a CopyFrom(byte[], int) method.

+ public InAddr (byte b0, byte b1, byte b2, byte b3)
+ {
+ s_addr = 0;
+ this[0] = b0;

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

This seems inefficient, as you'll be evaluating the ArgumentOufOfRangeException check 4 times, when you know you shouldn't need any.

I also wonder -- and I'm not sure how to verify :-( -- what byte ordering constraints there are, if any. I believe that you intend for s_addr to be in Network Byte Order...but should it be? I don't know what it should be, but this does give me pause.

@jonpryor

jonpryor Oct 7, 2015

Member

This seems inefficient, as you'll be evaluating the ArgumentOufOfRangeException check 4 times, when you know you shouldn't need any.

I also wonder -- and I'm not sure how to verify :-( -- what byte ordering constraints there are, if any. I believe that you intend for s_addr to be in Network Byte Order...but should it be? I don't know what it should be, but this does give me pause.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

This seems inefficient, as you'll be evaluating the ArgumentOufOfRangeException check 4 times, when you know you shouldn't need any.

Ok, I'll make sure the constructor doesn't do the range check.

I also wonder -- and I'm not sure how to verify :-( -- what byte ordering constraints there are, if any. I believe that you intend for s_addr to be in Network Byte Order...but should it be? I don't know what it should be, but this does give me pause.

Yes, s_addr is supposed to be network byte order. Mono_Posix_FromInAddr()/Mono_Posix_ToInAddr() assume that the layout of struct InAddr in C# and struct in_addr in C is the same.

I think that it should be network byte order, as most people think of an IP address not as a 32-bit number but as 4 8-bit numbers. One possibility would also be to make s_addr private (then the fact that it is stored in network byte order is no longer visible to the caller) and make callers rely on the setters/getters or the Copy*() methods.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

This seems inefficient, as you'll be evaluating the ArgumentOufOfRangeException check 4 times, when you know you shouldn't need any.

Ok, I'll make sure the constructor doesn't do the range check.

I also wonder -- and I'm not sure how to verify :-( -- what byte ordering constraints there are, if any. I believe that you intend for s_addr to be in Network Byte Order...but should it be? I don't know what it should be, but this does give me pause.

Yes, s_addr is supposed to be network byte order. Mono_Posix_FromInAddr()/Mono_Posix_ToInAddr() assume that the layout of struct InAddr in C# and struct in_addr in C is the same.

I think that it should be network byte order, as most people think of an IP address not as a 32-bit number but as 4 8-bit numbers. One possibility would also be to make s_addr private (then the fact that it is stored in network byte order is no longer visible to the caller) and make callers rely on the setters/getters or the Copy*() methods.

+ throw new ArgumentException ("buffer", "buffer.Length != 16");
+ addr0 = addr1 = 0;
+ fixed (ulong* ptr = &addr0)
+ Marshal.Copy (buffer, 0, (IntPtr) ptr, 16);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

See, here you copy them all at once instead of using the setter 16 times. :-) Yay!

@jonpryor

jonpryor Oct 7, 2015

Member

See, here you copy them all at once instead of using the setter 16 times. :-) Yay!

+ fixed (ulong* ptr = &addr0)
+ return ((byte*) ptr)[index];
+ }
+ set {

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Ditto previous questions/comments: should In6Addr be mutable? Should it have a setter?

I don't know. I'm not sure what the expected API usage should be.

@jonpryor

jonpryor Oct 7, 2015

Member

Ditto previous questions/comments: should In6Addr be mutable? Should it have a setter?

I don't know. I'm not sure what the expected API usage should be.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Ditto previous questions/comments: should In6Addr be mutable? Should it have a setter?

As for InAddr I'll be leaving it mutable and add a CopyFrom(byte[], int) method.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Ditto previous questions/comments: should In6Addr be mutable? Should it have a setter?

As for InAddr I'll be leaving it mutable and add a CopyFrom(byte[], int) method.

+
+ public static unsafe Cmsghdr Get (Msghdr msgh, long cmsg)
+ {
+ if (msgh.msg_control == null || msgh.msg_controllen > msgh.msg_control.Length)

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Should check that msgh isn't null before accessing members on it.

@jonpryor

jonpryor Oct 7, 2015

Member

Should check that msgh isn't null before accessing members on it.

+
+ public unsafe void StoreAt (Msghdr msgh, long cmsg)
+ {
+ if (msgh.msg_control == null || msgh.msg_controllen > msgh.msg_control.Length)

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Should check that msgh isn't null before accessing members on it.

@jonpryor

jonpryor Oct 7, 2015

Member

Should check that msgh isn't null before accessing members on it.

+ }
+ }
+
+ public static unsafe Cmsghdr Get (Msghdr msgh, long cmsg)

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

I'm not sure what Get() is supposed to be doing, or what POSIX API it corresponds to. Is it CMSG_FIRSTHDR()? CMSG_NXTHDR()? Something else entirely? (I'm inclined to say "something else entirely," but that doesn't help me in reviewing.)

A rename and/or comments may be helpful.

Looking at the corresponding unit tests...this is deep, deep, voodoo.

@jonpryor

jonpryor Oct 7, 2015

Member

I'm not sure what Get() is supposed to be doing, or what POSIX API it corresponds to. Is it CMSG_FIRSTHDR()? CMSG_NXTHDR()? Something else entirely? (I'm inclined to say "something else entirely," but that doesn't help me in reviewing.)

A rename and/or comments may be helpful.

Looking at the corresponding unit tests...this is deep, deep, voodoo.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm not sure what Get() is supposed to be doing, or what POSIX API it corresponds to. Is it CMSG_FIRSTHDR()? CMSG_NXTHDR()? Something else entirely? (I'm inclined to say "something else entirely," but that doesn't help me in reviewing.)

A rename and/or comments may be helpful.

The method is for retrieving a Cmsghdr structure from a buffer. In C you don't need a method for that, you simply cast the pointer you have to struct cmsghdr* (in C you would have pointers instead of offsets into the buffer).
I can add a comment explaining that, but I'm not sure regarding the best name for the method. ReadFromBuffer()?

Looking at the corresponding unit tests...this is deep, deep, voodoo.

Yes, but I'm not sure how to avoid that (the C API is rather ugly already).

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm not sure what Get() is supposed to be doing, or what POSIX API it corresponds to. Is it CMSG_FIRSTHDR()? CMSG_NXTHDR()? Something else entirely? (I'm inclined to say "something else entirely," but that doesn't help me in reviewing.)

A rename and/or comments may be helpful.

The method is for retrieving a Cmsghdr structure from a buffer. In C you don't need a method for that, you simply cast the pointer you have to struct cmsghdr* (in C you would have pointers instead of offsets into the buffer).
I can add a comment explaining that, but I'm not sure regarding the best name for the method. ReadFromBuffer()?

Looking at the corresponding unit tests...this is deep, deep, voodoo.

Yes, but I'm not sure how to avoid that (the C API is rather ugly already).

+ WithSocketPair ((inner_so1, inner_so2) => {
+ WithSocketPair ((so1, so2) => {
+ // Create two SCM_RIGHTS control messages
+ var cmsg = new byte[2 * Syscall.CMSG_SPACE (sizeof (int))];

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Using the example in the cmsg(3) man page, I'm not entirely sure that this is correct. (It's probably correct, but it might be implementation specific.)

       struct msghdr msg = {0};
       struct cmsghdr *cmsg;
       int myfds[NUM_FD]; /* Contains the file descriptors to pass. */
       union {
           /* ancillary data buffer, wrapped in a union in order to ensure
              it is suitably aligned */
           char buf[CMSG_SPACE(sizeof myfds)];
           struct cmsghdr align;
       } u;
       int *fdptr;

       msg.msg_control = u.buf;

msghdr1.msg_control (several lines below) is set to cmsg, meaning cmsg is equivalent to the man page's u.buf variable. Note that u.buf is a union between char buf[CMSG_SPACE(sizeof myfds)] and struct msghdr, meaning sizeof u is at least sizeof(struct msghdr) bytes in size, which would be Cmsghdr.Size bytes.

Not knowing what the value of Cmsghdr.Size, it is thus plausible (to idiots like me!) that Cmsghdr.Size is greater than what you actually allocated, new byte[2*Syscall.CMSG_SPACE(4)].

This doesn't leave me with a good feeling.

I have to wonder if following the POSIX API too closely here is actually a good thing. This seems brittle as hell, easy to get wrong, not at all obvious, and I want to run screaming in terror. :-(

@jonpryor

jonpryor Oct 7, 2015

Member

Using the example in the cmsg(3) man page, I'm not entirely sure that this is correct. (It's probably correct, but it might be implementation specific.)

       struct msghdr msg = {0};
       struct cmsghdr *cmsg;
       int myfds[NUM_FD]; /* Contains the file descriptors to pass. */
       union {
           /* ancillary data buffer, wrapped in a union in order to ensure
              it is suitably aligned */
           char buf[CMSG_SPACE(sizeof myfds)];
           struct cmsghdr align;
       } u;
       int *fdptr;

       msg.msg_control = u.buf;

msghdr1.msg_control (several lines below) is set to cmsg, meaning cmsg is equivalent to the man page's u.buf variable. Note that u.buf is a union between char buf[CMSG_SPACE(sizeof myfds)] and struct msghdr, meaning sizeof u is at least sizeof(struct msghdr) bytes in size, which would be Cmsghdr.Size bytes.

Not knowing what the value of Cmsghdr.Size, it is thus plausible (to idiots like me!) that Cmsghdr.Size is greater than what you actually allocated, new byte[2*Syscall.CMSG_SPACE(4)].

This doesn't leave me with a good feeling.

I have to wonder if following the POSIX API too closely here is actually a good thing. This seems brittle as hell, easy to get wrong, not at all obvious, and I want to run screaming in terror. :-(

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Not knowing what the value of Cmsghdr.Size, it is thus plausible (to idiots like me!) that Cmsghdr.Size is greater than what you actually allocated, new byte[2*Syscall.CMSG_SPACE(4)].

I don't think that's possible, as the space returned by CMSG_SPACE is supposed to contain a struct cmsghdr. Unfortunately, CMSG_SPACE does not seem to be part of POSIX (even though you need it to create a control message) :-$ However, it is described in RFC 2292 and it says "CMSG_SPACE() returns the space required by the object and its cmsghdr structure, including any padding needed to satisfy alignment requirements" which to me sounds like it should always return at least sizeof (struct cmsghdr).

I think the reason for the union in the C code is simply make ensure the alignment is sufficient.

I have to wonder if following the POSIX API too closely here is actually a good thing. This seems brittle as hell, easy to get wrong, not at all obvious, and I want to run screaming in terror. :-(

If you want I can try to add a high-level API for this. (Basically a abstract class ControlMessage + class FDControlMessage : ControlMessage and a way to convert between a byte[] and a List<ControlMessage>.)

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Not knowing what the value of Cmsghdr.Size, it is thus plausible (to idiots like me!) that Cmsghdr.Size is greater than what you actually allocated, new byte[2*Syscall.CMSG_SPACE(4)].

I don't think that's possible, as the space returned by CMSG_SPACE is supposed to contain a struct cmsghdr. Unfortunately, CMSG_SPACE does not seem to be part of POSIX (even though you need it to create a control message) :-$ However, it is described in RFC 2292 and it says "CMSG_SPACE() returns the space required by the object and its cmsghdr structure, including any padding needed to satisfy alignment requirements" which to me sounds like it should always return at least sizeof (struct cmsghdr).

I think the reason for the union in the C code is simply make ensure the alignment is sufficient.

I have to wonder if following the POSIX API too closely here is actually a good thing. This seems brittle as hell, easy to get wrong, not at all obvious, and I want to run screaming in terror. :-(

If you want I can try to add a high-level API for this. (Basically a abstract class ControlMessage + class FDControlMessage : ControlMessage and a way to convert between a byte[] and a List<ControlMessage>.)

+ msg_controllen = cmsg.Length,
+ };
+ long offset = 0;
+ hdr.StoreAt (msghdr1, offset);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

I'm still not sure what .StoreAt() (or .Get()) is supposed to mean, or what POSIX API it corresponds to.

@jonpryor

jonpryor Oct 7, 2015

Member

I'm still not sure what .StoreAt() (or .Get()) is supposed to mean, or what POSIX API it corresponds to.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm still not sure what .StoreAt() (or .Get()) is supposed to mean, or what POSIX API it corresponds to.

StoreAt() will write a struct cmsghdr to a byte[] buffer. Similar to Get(), this is possible in C simply using a cast but will require some wrapper in C#. Should I call it WriteToBuffer() instead?

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm still not sure what .StoreAt() (or .Get()) is supposed to mean, or what POSIX API it corresponds to.

StoreAt() will write a struct cmsghdr to a byte[] buffer. Similar to Get(), this is possible in C simply using a cast but will require some wrapper in C#. Should I call it WriteToBuffer() instead?

+ long size;
+ if (this != null && this.IsDynamic) {
+ fixed (byte* data = this.DynamicData ()) {
+ _SockaddrDynamic dyn = new _SockaddrDynamic (this, data, false);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Style: when using boolean parameters, use C#4 named parameters so we know WTF it's controlling:

new _SockaddrDynamic (this, data, useMaxLength:false);
@jonpryor

jonpryor Oct 7, 2015

Member

Style: when using boolean parameters, use C#4 named parameters so we know WTF it's controlling:

new _SockaddrDynamic (this, data, useMaxLength:false);
+
+ // This is a class which can store arbitrary sockaddrs, even if they are not known the the Mono.Unix wrapper or the family does not have a corresponding value in the UnixAddressFamily enumeration.
+ [CLSCompliant (false)]
+ public class SockaddrStorage : Sockaddr, IEquatable<SockaddrStorage> {

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Should this type be sealed?

@jonpryor

jonpryor Oct 7, 2015

Member

Should this type be sealed?

+ SockaddrIn6,
+
+ // Flag to indicate that this Sockaddr must be wrapped with a _SockaddrDynamic wrapper
+ MustBeWrapped = 0x8000,

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Unless you intend for "normal" people to use this value -- and I suspect you don't -- then please remove it. Instead of using SockaddrType, you can do

partial class Sockaddr {
    internal const SockaddrType MustBeWrapped = (SockaddrType) 0x8000;
}

This only slightly alters your code -- instead of SockaddrType.Sockaddr | SockaddrType.MustBeWrapped, you'd need SockaddrType.Sockaddr | Sockaddr.MustBeWrapped.

The benefit is that internal code is kept internal, instead of leaking into public API.

@jonpryor

jonpryor Oct 7, 2015

Member

Unless you intend for "normal" people to use this value -- and I suspect you don't -- then please remove it. Instead of using SockaddrType, you can do

partial class Sockaddr {
    internal const SockaddrType MustBeWrapped = (SockaddrType) 0x8000;
}

This only slightly alters your code -- instead of SockaddrType.Sockaddr | SockaddrType.MustBeWrapped, you'd need SockaddrType.Sockaddr | Sockaddr.MustBeWrapped.

The benefit is that internal code is kept internal, instead of leaking into public API.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

The enum SockaddrType is internal, so none of the SockaddrType values should be exposed.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

The enum SockaddrType is internal, so none of the SockaddrType values should be exposed.

+ return size;
+ }
+
+ internal bool IsDynamic {

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

I'm not sure what IsDynamic is supposed to mean, though I assume it has something to do with SockaddrType.MustBeWrapped:

// Flag to indicate that this Sockaddr must be wrapped with a _SockaddrDynamic wrapper

(I think part of my problem is I've been digging into the realm of System.Dynamic, so the appearance of Dynamic here is doing me no favors, as it's completely different.)

Perhaps IsWrapped would be better? Though I'm not sure of that name either. It would provide consistency with the MustBeWrapped flag...

@jonpryor

jonpryor Oct 7, 2015

Member

I'm not sure what IsDynamic is supposed to mean, though I assume it has something to do with SockaddrType.MustBeWrapped:

// Flag to indicate that this Sockaddr must be wrapped with a _SockaddrDynamic wrapper

(I think part of my problem is I've been digging into the realm of System.Dynamic, so the appearance of Dynamic here is doing me no favors, as it's completely different.)

Perhaps IsWrapped would be better? Though I'm not sure of that name either. It would provide consistency with the MustBeWrapped flag...

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm not sure what IsDynamic is supposed to mean, though I assume it has something to do with SockaddrType.MustBeWrapped:

It indicates whether the Sockaddr has to be wrapped using _DynamicSockaddr (i.e. whether the SockaddrType.MustBeWrapped flag is set).

Perhaps IsWrapped would be better? Though I'm not sure of that name either. It would provide consistency with the MustBeWrapped flag...

I'll rename it to NeedsWrapper. (IsWrapped is not really correct.)

Shouldn't be too important though, NeedsWrapper also is internal.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

I'm not sure what IsDynamic is supposed to mean, though I assume it has something to do with SockaddrType.MustBeWrapped:

It indicates whether the Sockaddr has to be wrapped using _DynamicSockaddr (i.e. whether the SockaddrType.MustBeWrapped flag is set).

Perhaps IsWrapped would be better? Though I'm not sure of that name either. It would provide consistency with the MustBeWrapped flag...

I'll rename it to NeedsWrapper. (IsWrapped is not really correct.)

Shouldn't be too important though, NeedsWrapper also is internal.

+ }
+ }
+ // This methods should only be called for SockaddrStorage and SockaddrUn where they are overwritten
+ internal virtual byte[] DynamicData () {

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Please try to follow Mono's coding convention: { on a new line for methods.

@jonpryor

jonpryor Oct 7, 2015

Member

Please try to follow Mono's coding convention: { on a new line for methods.

+ }
+ public SockaddrUn (string path, bool @abstract = false) : base (SockaddrType.SockaddrUn | SockaddrType.MustBeWrapped, UnixAddressFamily.AF_UNIX)
+ {
+ var bytes = UnixEncoding.Instance.GetBytes (path);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Don't forget to null-check the path parameter.

@jonpryor

jonpryor Oct 7, 2015

Member

Don't forget to null-check the path parameter.

+ [CLSCompliant (false)]
+ [StructLayout (LayoutKind.Sequential)]
+ public class SockaddrIn : Sockaddr, IEquatable<SockaddrIn> {
+ public UnixAddressFamily sin_family { // AF_INET

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Cute. :-)

@jonpryor

jonpryor Oct 7, 2015

Member

Cute. :-)

+ [StructLayout (LayoutKind.Sequential)]
+ public class SockaddrIn : Sockaddr, IEquatable<SockaddrIn> {
+ public UnixAddressFamily sin_family { // AF_INET
+ get {

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Style: You can use a single-line form here:

get { return sa_family; }
set { sa_family = value; }
@jonpryor

jonpryor Oct 7, 2015

Member

Style: You can use a single-line form here:

get { return sa_family; }
set { sa_family = value; }
support/sys-socket.c
+ struct Mono_Posix__SockaddrDynamic* destination_dyn;
+
+ if (!destination)
+ return 0;

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Is this really a "no error" situation?

@jonpryor

jonpryor Oct 7, 2015

Member

Is this really a "no error" situation?

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Is this really a "no error" situation?

Yes. Some calls (e.g. recvfrom()) allow the address parameter to be NULL. In this case, there is no need to copy anything, and NULL will be passed to the syscall (ALLOC_SOCKADDR initializes addr with NULL in this case).

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Is this really a "no error" situation?

Yes. Some calls (e.g. recvfrom()) allow the address parameter to be NULL. In this case, there is no need to copy anything, and NULL will be passed to the syscall (ALLOC_SOCKADDR initializes addr with NULL in this case).

support/sys-socket.c
+ break;
+
+ case Mono_Posix_SockaddrType_SockaddrStorage:
+ destination_dyn = ((struct Mono_Posix__SockaddrDynamic*) destination);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

I don't understand why SockaddrType.SockaddrStorage and SockaddrType.SockaddrUn is assumed to be a _SockaddrDynamic instance, while the other SockaddrType values aren't. (I haven't verified if this is the case.)

The inconsistency here is odd.

@jonpryor

jonpryor Oct 7, 2015

Member

I don't understand why SockaddrType.SockaddrStorage and SockaddrType.SockaddrUn is assumed to be a _SockaddrDynamic instance, while the other SockaddrType values aren't. (I haven't verified if this is the case.)

The inconsistency here is odd.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

SockaddrStorage and SockaddrUn require a dynamic amount of memory, the other types always require the same amount.
The C# code will create a _SockaddrDynamic instance if the SockaddrType has the MustBeWrapped flag set. (And in case the C# code should forget to do that, the switch will fall through to the default: case.)

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

SockaddrStorage and SockaddrUn require a dynamic amount of memory, the other types always require the same amount.
The C# code will create a _SockaddrDynamic instance if the SockaddrType has the MustBeWrapped flag set. (And in case the C# code should forget to do that, the switch will fall through to the default: case.)

support/sys-socket.c
+ return -1;
+ }
+ break;
+

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

Why is nothing done for SockaddrType.Sockaddr?

@jonpryor

jonpryor Oct 7, 2015

Member

Why is nothing done for SockaddrType.Sockaddr?

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Because the only field of struct sockaddr (sa_family) is copied later in the code which is common for all Sockaddr* structures.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

Because the only field of struct sockaddr (sa_family) is copied later in the code which is common for all Sockaddr* structures.

+ // htonl(3)
+ // uint32_t htonl(uint32_t hostlong);
+ [DllImport (LIBC)]
+ public static extern uint htonl(uint hostlong);

This comment has been minimized.

@jonpryor

jonpryor Oct 7, 2015

Member

I thought mentioned elsewhere/previously, but I'm not sure we need to bind these as identical functionality is available from IPAddress.NetworkToHostOrder(), except the methods here operate on unsigned t types.

@jonpryor

jonpryor Oct 7, 2015

Member

I thought mentioned elsewhere/previously, but I'm not sure we need to bind these as identical functionality is available from IPAddress.NetworkToHostOrder(), except the methods here operate on unsigned t types.

This comment has been minimized.

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

But the Unix socket API uses unsigned values, meaning that the user would have to do ugly casts (unless the API is changed to use host byte order for all values, similar to the System.Net.Sockets API, but I think you've decided against that).

I could change the calls to do the casts and call to IPAddress.NetworkToHostOrder(), of course (which might be faster because the JIT can inline everything).

@steffen-kiess

steffen-kiess Oct 7, 2015

Contributor

But the Unix socket API uses unsigned values, meaning that the user would have to do ugly casts (unless the API is changed to use host byte order for all values, similar to the System.Net.Sockets API, but I think you've decided against that).

I could change the calls to do the casts and call to IPAddress.NetworkToHostOrder(), of course (which might be faster because the JIT can inline everything).

@jonpryor

This comment has been minimized.

Show comment
Hide comment
@jonpryor

jonpryor Jan 8, 2016

Member

What's the difference between PR 2006 and PR 2007 (this one)? Both alter configure.ac to check for HAVE_STRUCT_SOCKADDR, among others. Both add Mono_Posix_FromInAddr() to NativeConvert.cs.

Comparing 2006.patch to 2097.patch shows that only commit 8b6f897 is the main difference; 2379 lines of 3538 are identical.

This pull request depends on #2006

That's kinda confusing when this PR includes the contents of #2006.

I'm not quite sure what to make of that. :-(

Member

jonpryor commented Jan 8, 2016

What's the difference between PR 2006 and PR 2007 (this one)? Both alter configure.ac to check for HAVE_STRUCT_SOCKADDR, among others. Both add Mono_Posix_FromInAddr() to NativeConvert.cs.

Comparing 2006.patch to 2097.patch shows that only commit 8b6f897 is the main difference; 2379 lines of 3538 are identical.

This pull request depends on #2006

That's kinda confusing when this PR includes the contents of #2006.

I'm not quite sure what to make of that. :-(

@steffen-kiess

This comment has been minimized.

Show comment
Hide comment
@steffen-kiess

steffen-kiess Jan 11, 2016

Contributor

What's the difference between PR 2006 and PR 2007 (this one)? Both alter configure.ac to check for HAVE_STRUCT_SOCKADDR, among others. Both add Mono_Posix_FromInAddr() to NativeConvert.cs.

Comparing 2006.patch to 2097.patch shows that only commit 8b6f897 is the main difference; 2379 lines of 3538 are identical.

This pull request depends on #2006

That's kinda confusing when this PR includes the contents of #2006.

This PR contains 2 commits, the first one is the same as the one in #2006. I don't think this is possible to do this in a different way in github. As long as the first commit (i.e. #2006) is not in the mono master branch, github will always show both commits. Once #2006 is accepted, this PR should change to only showing the "new" commit. (Or it would, but I'll have to make some changes to #2006 and then the commit IDs won't match anymore.)

You can either comment only on the second commit or ignore this PR until #2006 is accepted (then I'll also update this PR to include the changes you've requested in #2006).

Contributor

steffen-kiess commented Jan 11, 2016

What's the difference between PR 2006 and PR 2007 (this one)? Both alter configure.ac to check for HAVE_STRUCT_SOCKADDR, among others. Both add Mono_Posix_FromInAddr() to NativeConvert.cs.

Comparing 2006.patch to 2097.patch shows that only commit 8b6f897 is the main difference; 2379 lines of 3538 are identical.

This pull request depends on #2006

That's kinda confusing when this PR includes the contents of #2006.

This PR contains 2 commits, the first one is the same as the one in #2006. I don't think this is possible to do this in a different way in github. As long as the first commit (i.e. #2006) is not in the mono master branch, github will always show both commits. Once #2006 is accepted, this PR should change to only showing the "new" commit. (Or it would, but I'll have to make some changes to #2006 and then the commit IDs won't match anymore.)

You can either comment only on the second commit or ignore this PR until #2006 is accepted (then I'll also update this PR to include the changes you've requested in #2006).

@steffen-kiess

This comment has been minimized.

Show comment
Hide comment
@steffen-kiess

steffen-kiess Jan 15, 2016

Contributor

I like this plan. :-)

Please do that.

I've implemented it in #2006. (I'll update this PR once #2006 is accepted.)

Contributor

steffen-kiess commented Jan 15, 2016

I like this plan. :-)

Please do that.

I've implemented it in #2006. (I'll update this PR once #2006 is accepted.)

[Mono.Posix] Add support for sending and receiving socket control mes…
…sages

Add wrappers for struct cmsghdr, the macros for manipulating control
messages (CMSG_*) and recvmsg() and sendmsg().
@steffen-kiess

This comment has been minimized.

Show comment
Hide comment
@steffen-kiess

steffen-kiess Jan 19, 2016

Contributor

If updated the pull request:

  • The changes to #2006 have been integrated
  • Everything has been rebased to current mono master
  • ArgumentException argument order has been fixed
  • 249ee06 added Mono_Unix_VersionString() with a return type of const char*, but create-native-map thinks it should be void*, so I've changed it to void*.
  • d243b91 added a check for accept4() and returned EINVAL on error. I've changed that to not generating the wrapper method at all (i.e. calling it will cause an EntryPointNotFoundException, which AFAIK is the preferred way of doing that).

Might this be "better" as an instance method on Msghdr? I'm not sure...

(Regarding CMSG_FIRSTHDR)
I'm also not sure. Currently the classes in Mono.Unix.Native do not export C functions/macros as instance methods. If you think I should change this to an instance method, should I also move Cmsghdr.ReadFromBuffer() and Cmsghdr.WriteToBuffer() to Msghdr?

Also, should I add a high-level wrapper for control messages? (If yes, in this commit or in another pull request?)

Contributor

steffen-kiess commented Jan 19, 2016

If updated the pull request:

  • The changes to #2006 have been integrated
  • Everything has been rebased to current mono master
  • ArgumentException argument order has been fixed
  • 249ee06 added Mono_Unix_VersionString() with a return type of const char*, but create-native-map thinks it should be void*, so I've changed it to void*.
  • d243b91 added a check for accept4() and returned EINVAL on error. I've changed that to not generating the wrapper method at all (i.e. calling it will cause an EntryPointNotFoundException, which AFAIK is the preferred way of doing that).

Might this be "better" as an instance method on Msghdr? I'm not sure...

(Regarding CMSG_FIRSTHDR)
I'm also not sure. Currently the classes in Mono.Unix.Native do not export C functions/macros as instance methods. If you think I should change this to an instance method, should I also move Cmsghdr.ReadFromBuffer() and Cmsghdr.WriteToBuffer() to Msghdr?

Also, should I add a high-level wrapper for control messages? (If yes, in this commit or in another pull request?)

@jonpryor

This comment has been minimized.

Show comment
Hide comment
@jonpryor

jonpryor Jan 19, 2016

Member

build

Member

jonpryor commented Jan 19, 2016

build

@brookpatten

This comment has been minimized.

Show comment
Hide comment
@brookpatten

brookpatten Jan 22, 2016

Hi Steffen, thank you for contributing/sharing this. I have been following your progress with interest and borrowing liberally for a project of my own. I wanted to ask you, is your intent to add unix file descriptor support to dbus-sharp?

Hi Steffen, thank you for contributing/sharing this. I have been following your progress with interest and borrowing liberally for a project of my own. I wanted to ask you, is your intent to add unix file descriptor support to dbus-sharp?

This comment has been minimized.

Show comment
Hide comment
@steffen-kiess

steffen-kiess Jan 22, 2016

Owner

I wanted to ask you, is your intent to add unix file descriptor support to dbus-sharp?

Yes, that's what I intent to do.

Owner

steffen-kiess replied Jan 22, 2016

I wanted to ask you, is your intent to add unix file descriptor support to dbus-sharp?

Yes, that's what I intent to do.

This comment has been minimized.

Show comment
Hide comment
@brookpatten

brookpatten Jan 22, 2016

Excellent, news! I thought so. I implemented it using your mono patch (badly, needs lots of work which I plan to do this weekend) so I am interested in seeing what you come up with as well.

Excellent, news! I thought so. I implemented it using your mono patch (badly, needs lots of work which I plan to do this weekend) so I am interested in seeing what you come up with as well.

jonpryor added a commit that referenced this pull request Jan 25, 2016

Merge pull request #2097 from steffen-kiess/posix-sockets-3
 [Mono.Posix] Add support for sending and receiving socket control messages

@jonpryor jonpryor merged commit 44e2c53 into mono:master Jan 25, 2016

3 of 5 checks passed

ARM hard float Linux Build finished. 37967 tests run, 891 skipped, 0 failed.
Details
ARM soft float Linux Build finished. 43949 tests run, 1025 skipped, 0 failed.
Details
AMD64 Linux Build finished. 44411 tests run, 1017 skipped, 0 failed.
Details
i386 Linux Build finished. 44411 tests run, 1016 skipped, 0 failed.
Details
i386 Windows Build finished. No test results found.
Details

akoeplinger added a commit to akoeplinger/mono that referenced this pull request Jan 26, 2016

[Mono.Posix] Disable new ControlMsg() test that fails on OSX for now
Added with mono#2097. Fails on Jenkins:

``
Test Case Failures:
1) MonoTests.Mono.Unix.Native.SocketTest.ControlMsg : System.ArgumentException : Invalid argument
  ----> Mono.Unix.UnixIOException : Invalid argument [EINVAL].
at Mono.Unix.UnixMarshal.ThrowExceptionForLastError () [0x00000] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Mono.Unix/UnixMarshal.cs:460
at MonoTests.Mono.Unix.Native.SocketTest+<ControlMsg>c__AnonStorey2.<>m__0 (Int32 so1, Int32 so2) [0x0018f] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:778
at MonoTests.Mono.Unix.Native.SocketTest.WithSocketPair (System.Action`2 f) [0x00025] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:68
at MonoTests.Mono.Unix.Native.SocketTest.<ControlMsg>m__9 (Int32 inner_so1, Int32 inner_so2) [0x00014] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:739
at MonoTests.Mono.Unix.Native.SocketTest.WithSocketPair (System.Action`2 f) [0x00025] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:68
at MonoTests.Mono.Unix.Native.SocketTest.ControlMsg () [0x00000] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:738
at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00038] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/corlib/System.Reflection/MonoMethod.cs:295
--UnixIOException
```

akoeplinger added a commit to akoeplinger/mono that referenced this pull request Jan 26, 2016

[Mono.Posix] Disable new ControlMsg() test that fails on OSX for now
Added with mono#2097. Fails on Jenkins:

```
Test Case Failures:
1) MonoTests.Mono.Unix.Native.SocketTest.ControlMsg : System.ArgumentException : Invalid argument
  ----> Mono.Unix.UnixIOException : Invalid argument [EINVAL].
at Mono.Unix.UnixMarshal.ThrowExceptionForLastError () [0x00000] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Mono.Unix/UnixMarshal.cs:460
at MonoTests.Mono.Unix.Native.SocketTest+<ControlMsg>c__AnonStorey2.<>m__0 (Int32 so1, Int32 so2) [0x0018f] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:778
at MonoTests.Mono.Unix.Native.SocketTest.WithSocketPair (System.Action`2 f) [0x00025] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:68
at MonoTests.Mono.Unix.Native.SocketTest.<ControlMsg>m__9 (Int32 inner_so1, Int32 inner_so2) [0x00014] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:739
at MonoTests.Mono.Unix.Native.SocketTest.WithSocketPair (System.Action`2 f) [0x00025] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:68
at MonoTests.Mono.Unix.Native.SocketTest.ControlMsg () [0x00000] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/Mono.Posix/Test/Mono.Unix.Native/SocketTest.cs:738
at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00038] in /Users/builder/jenkins/workspace/test-mono-mainline@2/label/osx-i386/mcs/class/corlib/System.Reflection/MonoMethod.cs:295
--UnixIOException
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment