Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break up setsockopt support and its TCP options. #7

Closed
wants to merge 1 commit into from

Conversation

haesbaert
Copy link
Contributor

This makes sockopt available to OS X and very likely to other BSD systems, more
especifically, OS X does not have TCP_KEEPIDLE, but has all the others.

If we can't detect the TCP option, we define the value to -1, then we fake a
ENOPROTOOPT if the user passed an option defined to -1.

The risk, although virtually impossible, is that there is a unix system actually
using -1 to the value of those tcp options.

checking VMSPLICE............failed
-checking SOCKOPT.............failed
+checking SOCKOPT.............ok
+checking TCP_KEEPCNT.........ok
+checking TCP_KEEPIDLE........failed
+checking TCP_KEEPINTVL.......ok
checking POLL................ok

@haesbaert
Copy link
Contributor Author

Ack on the coding style.

I was rereading the code, if we raise Not_available at the setsockopt option, it would be the first raising of Not_available from C code, also the first case of something inside ExtUnixSpecific that can raise Not_available.

This premise would have to be changed as well:

[ExtUnix.Specific] includes only functions available on the current
platform and will not raise [Not_available].
Note that libc wrappers underlying [ExtUnix.Specific] functions may still raise
[ENOSYS] (Not implemented) error even though the function is available. *)

Still wanna do it ? Fine for me.

@haesbaert
Copy link
Contributor Author

I'm having some issues registering the exception back to C, the code in question doesn't run:

let () = Callback.register_exception "Not_available" (Not_available "nothing")

I've tried adding it right after the Not_available definition, but it doesn't run. When I add that line to my test program, everything works and I can call the exception from C code.

@ygrek
Copy link
Owner

ygrek commented May 28, 2015

The plan is to add preprocessing so that e.g. TCP_KEEPIDLE will not be visible in Specific module at all if it is not detected, and Not_available will be raised only in module All as expected.. As for exception not registering - this probably has to do with ExtUnix modules not having mli files, this is the known case when ocamlopt inlines external function calls without evaluating toplevel module definitions... One possible solution would be to make setsockopt first register exception and then call external function, but that's ugly..

@haesbaert
Copy link
Contributor Author

What if we cheat, we raise "invalid_arg" on the external setsockopt/getsockopt, and then on the ocaml setsockopt/getsockopt we convert the invalid_arg into a Not_available. Just reread the code, invalid_arg is already raised on an invalid value for k, which should also be impossible. But at any rate, we could still convert the Unix errno=ENOPROTOOPT, or use some other errno as the indicator, we catch it and convert to a Not_available.

@ygrek
Copy link
Owner

ygrek commented May 28, 2015

Yes, I agree, seems less hacky way to do it. Not_found should be ok I guess?

@haesbaert
Copy link
Contributor Author

Right on, not found should be fine.
On 28 May 2015 21:56, "ygrek" notifications@github.com wrote:

Yes, I agree, seems less hacky way to do it. Not_found should be ok I
guess?


Reply to this email directly or view it on GitHub
#7 (comment).

@haesbaert
Copy link
Contributor Author

Take a look, if that is fine, please don't merge the pull request, I want to rebase and clean the history.

@@ -772,11 +772,22 @@ type socket_int_option =
keepalive probes, if the socket option SO_KEEPALIVE has been set on this socket *)
| TCP_KEEPINTVL (** The time (in seconds) between individual keepalive probes *)

external setsockopt_int_ext : Unix.file_descr -> socket_int_option -> int -> unit = "caml_extunix_setsockopt_int"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename as setsockopt_int so that it is shadowed and cannot be used accidentaly

@ygrek
Copy link
Owner

ygrek commented Jun 22, 2015

Ah, missed the latest commit.

please don't merge the pull request, I want to rebase and clean the history.

Looks fine, should I merge it?

@ygrek ygrek self-assigned this Jun 22, 2015
@haesbaert
Copy link
Contributor Author

Awesome, thanks, I've more stuff to send.
Blargh, sorry for the getsock_opt_int fuck up, embarassing mistake.
Will do the rebase/clean up when I get home

This makes sockopt available to OS X and very likely to other BSD systems, more
especifically, OS X does not have TCP_KEEPIDLE, but has all the others.

If we can't detect the TCP option, we define the value to -1, then we fake a
ENOPROTOOPT if the user passed an option defined to -1.

The risk, although virtually impossible, is that there is a unix system actually
using -1 to the value of those tcp options.

 checking VMSPLICE............failed
-checking SOCKOPT.............failed
+checking SOCKOPT.............ok
+checking TCP_KEEPCNT.........ok
+checking TCP_KEEPIDLE........failed
+checking TCP_KEEPINTVL.......ok
 checking POLL................ok
@ygrek
Copy link
Owner

ygrek commented Jul 5, 2015

Updated comment and committed. Thanks!

@ygrek ygrek closed this Jul 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants