Application-friendly sockopt interface

[kellymclaughlin]

This PR provides an optional way for applications to specify the level and option_name parameters to setsockopt and getsockopt as atoms representing the names of the OS-level defines. It shifts the burden of cross-platform compatability for those function calls from the application to the procket library.

The motivation for this comes from trying to get our application that uses procket to build on SmartOS in addition to Linux. We also have some developers who use OSX who would like the application to build in their local development environment. We have an ugly case statement to set the level and option_name values we need for each OS, but that is brittle and it is something better done once by the library than by each application using the library.

We have this branch successfully building on Arch Linux, Debian 7, SmartOS, and OSX. The set of supported levels and option names is not exhaustive, but we felt it was a good start and could be easily extended to meet other users' needs. The existing interface is not broken by the change so existing applications will still work without change.

[msantos]

Hey Kelly! This is great! I was just thinking about doing something similar for the socket protocol and family constants. I agree with everything you wrote. It's a burden to force the portability handling onto the user of the library.

I'd like to be able to re-use your code to lookup other values like socket type, family and protocol. For discussion, here is what I was thinking of:

https://github.com/msantos/procket/tree/helium-friendly-sockopt-interface-wip

Changes:

• since atoms are limited to 255 bytes, I think we can just allocate on the stack and avoid the checks around memory allocations.
• moved out the constant lookups to a new function and switched to searching through a table. If this is too slow, the values could be cached. For example, maybe an erlang module could be generated using the NIF at compile time.

Please let me know what you think!

Some other questions:

• Should an invalid constant throw badarg or return {error,einval}? An invalid level or optname atom results in badarg. An invalid level/optname integer returns {error,einval}.

  I think it is more correct to throw badarg but I'm wandering if it will make portable code more difficult. For example:

% Random optname that Solaris doesn't support
try procket:setsockopt(FD, 'SOL_SOCKET', 'SO_REUSEPORT', <<0:32>>) of
    {ok,_} -> ok;
    {error, _} = Error -> Error
catch
   error:badarg -> ok
end.

% Vs
case procket:setsockopt(FD, 'SOL_SOCKET', 'SO_REUSEPORT', <<0:32>>) of
   {ok, _} -> ok;
   {error, einval} -> ok;
   {error, _} = Error -> Error
end.

The downside is an invalid atom like an optname with a typo ('SO_REUSEPOR') is indistinguishable from the OS returning EINVAL to the call.

• 'SOL_SOCKET' vs sol_socket. I prefer the quoted, upper case atoms but the erlang stdlib usually uses lower case like for errno: eperm, etimedout, ...

Thanks again, I really appreciate this patch! And sorry for all the questions.

[kellymclaughlin]

Thanks for the great feedback. I really like your changes. The table lookup is much cleaner to look at. It is unfortunate to have to scan the entire table in the worst case, but to start with since there are less than 100 values currently specified it is probably not a huge problem. Another optimization path might be to generate a trie or a hash table for efficient lookups, but that could be done later if it really becomes an issue or someone has a real inclination to do so.

Should an invalid constant throw badarg or return {error,einval}? An invalid level or optname atom results in badarg. An invalid level/optname integer returns {error,einval}.

This is a tough call, but I think I would lean towards just returning einval or whatever errno is generated by the C function call and let the apps decide from there. To me it seems more in the spirit of procket in that it gives you the same experience as if you were using the C functions directly. One risk I guess is that someone doesn't check error codes and perhaps the app carries along and then crashes in a different place due to an unchecked error and it makes it more difficult to debug the problem whereas as a badarg exception being thrown might make the problem more obvious, but personally I think I still prefer returning the errno to an exception.

'SOL_SOCKET' vs sol_socket. I prefer the quoted, upper case atoms but the erlang stdlib usually uses lower case like for errno: eperm, etimedout, ...

+1 to quoted upper-case. I like that better as well.

Let me know if there is anything else I can do to help with this. Thanks!

[msantos]

On Wed, Jun 03, 2015 at 10:26:03AM -0700, Kelly McLaughlin wrote:

Thanks for the great feedback. I really like your changes. The table lookup is much cleaner to look at. It is unfortunate to have to scan the entire table in the worst case, but to start with since there are less than 100 values currently specified it is probably not a huge problem. Another optimization path might be to generate a trie or a hash table for efficient lookups, but that could be done later if it really becomes an issue or somehow has a real inclination to do so.

Agreed. Trie/hash lookups are a good idea, I'll look into them. I'm also toying with generating an erlang module with the constants at compile time. This should be faster since we won't have to go from erlang into the NIF.

Should an invalid constant throw badarg or return {error,einval}? An invalid level or optname atom results in badarg. An invalid level/optname integer returns {error,einval}.

This is a tough call, but I think I would lean towards just returning einval or whatever errno is generated by the C function call and let the apps decide from there. To me it seems more in the spirit of procket in that it gives you the same experience as if you were using the C functions directly. One risk I guess is that someone doesn't check error codes and perhaps the app carries along and then crashes in a different place due to an unchecked error and it makes it more difficult to debug the problem whereas as a badarg exception being thrown might make the problem more obvious, but personally I think I still prefer returning the errno to an exception.

Ok, sounds good. I'll modify the code to return {error,unsupported} if passed in an invalid atom.

'SOL_SOCKET' vs sol_socket. I prefer the quoted, upper case atoms but the erlang stdlib usually uses lower case like for errno: eperm, etimedout, ...

+1 to quoted upper-case. I like that better as well.

Done!

Let me know if there is anything else I can do to help with this. Thanks!

Thanks again, will do!

[jaredmorrow]

Thanks @msantos!