Changed SafeFileDescriptorHandle to SafeHandle

[Vincent-X-Zhang]

Changed SafeFileDescriptorHandle to SafeHandle in the most of NativeMethods

[derek-will]

Interesting! Leveraging the base class SafeHandle of SafeFileDescriptorHandle everywhere possible. I like that this change, although large in scale, is essentially backwards compatible. I am considering the pros and cons.

[Vincent-X-Zhang]

Interesting! Leveraging the base class SafeHandle of SafeFileDescriptorHandle everywhere possible. I like that this change, although large in scale, is essentially backwards compatible. I am considering the pros and cons.

There are three reasons to support modification:

1. SafeHandle is designed to manage IntPtr more safely, and SafeFileDescriptorHandle basically just override a method and a property. So, It's actually quite safe to make this change.
2. Users can customize SafeHandle.
3. More consistent with the design pattern.

By the way, the repo have any discussions or discord? I have some new features want to discuss with you all.

[derek-will]

SafeHandle is designed to manage IntPtr more safely, and SafeFileDescriptorHandle basically just override a method and a property. So, It's actually quite safe to make this change.

SafeFileDescriptorHandle is derived from SafeHandleMinusOneIsInvalid which is derived from SafeHandle. It just provides a more appropriate validity condition check as file descriptors are non-negative integer values where -1 represents an error. There were also a few quirks with the default implementation provided by SafeHandleMinusOneIsInvalid where it would falsely indicate that the handle was valid post-closure which I addressed. Other than that, it provides an implementation for freeing the handle which is the most critical.

[derek-will]

Users can customize SafeHandle.

Users should always be using the SafeFileDescriptorHandle returned from either Socket() or EpollCreate() functions in LibcNativeMethods class.

I am unsure of what a customization use-case might be. Any proposals?

[derek-will]

More consistent with the design pattern.

I've been considering this. Parameter Design Guidelines on Microsoft's site state the following:

✔️ DO use the least derived parameter type that provides the functionality required by the member.

For example, suppose you want to design a method that enumerates a collection and prints each item to the console. Such a method should take IEnumerable as the parameter, not ArrayList or IList, for example.

SafeHandle and many of the SafeHandle derived classes do not provide the functionality required - However, SafeFileDescriptorHandle does provide the functionality required. In other words, a handle to a file descriptor is what is absolutely necessary in all function calls. Meanwhile, a handle to a process access token via SafeAccessTokenHandle or a handle to a pipe via SafePipeHandle would be wrong to use in those function calls, but permissible via the revised function signatures. I find this problematic even though polymorphism and Liskov's Substitution Principle will allow for it here.

I don't anticipate a reason to need any other handle types other than SafeFileDescriptorHandle which is only required because SafeSocketHandle is not supported in .NET Standard 2.0 (or any other version for that matter). My thoughts on this are best summarized as...

Users should always be using the SafeFileDescriptorHandle returned from either Socket() or EpollCreate() functions in LibcNativeMethods class.

[derek-will]

By the way, the repo have any discussions or discord? I have some new features want to discuss with you all.

It has just been me working on this project. I have received some excellent ideas and feedback through Issues and Pull Requests on GitHub. I have never used the "Discussions" feature, but I am open to using it!

[Vincent-X-Zhang]

Sorry, I may not have made myself clear.

Users can customize SafeHandle.

I mean the user can inherit SafeHandle or SafeHandleMinusOneIsInvalid and thus customize it.

Other than that, it provides an implementation for freeing the handle which is the most critical.

SafeHandle inherit the interface IDisposable, so, any classes which inherit SafeHandle can dispose themselves.

By the way, i more recommanded to hide the P/Invoke methods:

[DllImport("libc", EntryPoint = "write", SetLastError = true)]
internal unsafe static extern nint Write(int fd, void* buf, nuint size);

public static nint Write(int fd, ReadOnlySpan<byte> buf, nuint size)
{
  if (size > (nuint)buf.Length)
  throw new ArgumentOutOfRangeException(nameof(size), "Count exceeds buffer length.");
  unsafe
  {
    fixed (byte* ptr = buf)
    {
      return Write(fd, ptr, size);
    }
  }
}

public static nint Write<T>(int fd, ref T buf, nuint size) where T : struct
{
  unsafe 
  {
    fixed (T* ptr = &buf)
    {
      return Write(fd, ptr, size);
    }
  }
}

[derek-will]

I mean the user can inherit SafeHandle or SafeHandleMinusOneIsInvalid and thus customize it.

This is already provided to the library user via SafeFileDescriptorHandle. I am unsure if any scenario exists where a user would need to customize beyond that implementation.

SafeHandle inherit the interface IDisposable, so, any classes which inherit SafeHandle can dispose themselves.

For SafeHandle derived classes, that is only part of it. In the "Notes to Implementers" section of SafeHandle, Microsoft states the following:

When you inherit from SafeHandle, you must override the following members: IsInvalid and ReleaseHandle().

You should also provide a public parameterless constructor that calls the base constructor with a value that represents an invalid handle value, and a Boolean value indicating whether the native handle is owned by the SafeHandle and consequently should be freed when that SafeHandle has been disposed.

[derek-will]

By the way, i more recommanded to hide the P/Invoke methods:

[DllImport("libc", EntryPoint = "write", SetLastError = true)]
internal unsafe static extern nint Write(int fd, void* buf, nuint size);

public static nint Write(int fd, ReadOnlySpan<byte> buf, nuint size)
{
  if (size > (nuint)buf.Length)
  throw new ArgumentOutOfRangeException(nameof(size), "Count exceeds buffer length.");
  unsafe
  {
    fixed (byte* ptr = buf)
    {
      return Write(fd, ptr, size);
    }
  }
}

public static nint Write<T>(int fd, ref T buf, nuint size) where T : struct
{
  unsafe 
  {
    fixed (T* ptr = &buf)
    {
      return Write(fd, ptr, size);
    }
  }
}

It is a stated goal in the README that "Using this library you can either use the higher level classes or the lower level libc P/Invoke calls directly." The user can either use the libc P/Invokes directly or they can use the higher level classes that hide those details.

Some examples of the aforementioned "hiding" in action:

• SocketCANSharp/src/SocketCANSharp/Network/RawCanSocket.cs

  Line 223 in 08d2e77

  public int Write(CanFrame canFrame)

• SocketCANSharp/src/SocketCANSharp/Network/RawCanSocket.cs

  Line 601 in 08d2e77

  private int Read<T>(out T frame, out bool txSuccess, out bool localhost)

• SocketCANSharp/src/SocketCANSharp/Network/RawCanSocket.cs

  Line 384 in 08d2e77

  private void SetRawCanFilters(CanFilter[] canFilterArray)

• SocketCANSharp/src/SocketCANSharp/Network/RawCanSocket.cs

  Line 395 in 08d2e77

  private CanFilter[] GetRawCanFilters()

[Vincent-X-Zhang]

It is a stated goal in the README that "Using this library you can either use the higher level classes or the lower level libc P/Invoke calls directly." The user can either use the libc P/Invokes directly or they can use the higher level classes that hide those details.

It's true that u hided those wappers in the different positions. But as i mentioned in issues, there are too many Write functions in the LibcNativeMethods. And obviously it will get more and more while the repo support more protocols. I mean, why not add a uniform wapper and let other classes impl their own way? :)

More Information: LibraryImport