Skip to content

Fix building on OpenBSD, allow one-way shutdown#63

Merged
anmonteiro merged 1 commit into
savonet:masterfrom
madroach:master
Sep 26, 2022
Merged

Fix building on OpenBSD, allow one-way shutdown#63
anmonteiro merged 1 commit into
savonet:masterfrom
madroach:master

Conversation

@madroach
Copy link
Copy Markdown

@madroach madroach commented Jun 21, 2020

  • To send an EOF over ssl you need to be able to initiate a one-way shutdown
  • Fix building against LibreSSL on OpenBSD

See also ocsigen/lwt_ssl#2 and mirage/ocaml-conduit#319

@madroach
Copy link
Copy Markdown
Author

I don't care about the close_notify change so much anymore, but please merge the compatibility changes to support LibreSSL.

@smorimoto
Copy link
Copy Markdown

@smimram @toots Could you check this as well?

Comment thread src/ssl.mli Outdated

(** Close an SSL connection. *)
(** send close notify to the peer. This is SSL_shutdown(3) *)
val close_notify : socket -> int
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The API follows the general convention that the return value is never exposed, because it is quite meaningless in OCaml. A boolean indicating whehter the shutdown is finished or not (and exceptions in other cases) would be better.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I changed it to bool.

Comment thread src/ssl.mli Outdated
val flush : socket -> unit

(** Close an SSL connection. *)
(** send close notify to the peer. This is SSL_shutdown(3) *)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
(** send close notify to the peer. This is SSL_shutdown(3) *)
(** Send close notify to the peer. This is SSL_shutdown(3). *)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

@madroach madroach force-pushed the master branch 2 times, most recently from 422a115 to 9bad44b Compare January 6, 2022 07:24
this is needed for sending EOF, aka one-way shutdown.
Comment thread src/ssl.ml

let rec shutdown sock =
if not (close_notify sock)
then shutdown sock
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be close_notify to prevent unbounded recursion?

Suggested change
then shutdown sock
then close_notify sock

@anmonteiro anmonteiro merged commit a346580 into savonet:master Sep 26, 2022
@anmonteiro anmonteiro mentioned this pull request Sep 26, 2022
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Oct 20, 2022
CHANGES:

- Add `Ssl.close_notify` to perform a one-way shutdown (savonet/ocaml-ssl#63, savonet/ocaml-ssl#96).
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.

4 participants