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.

Sign up for GitHub

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

Jump to bottom

fix issue #103 for output_string, output_char and input_int #116

Merged
merged 4 commits into from May 28, 2023
Merged

fix issue #103 for output_string, output_char and input_int #116

merged 4 commits into from May 28, 2023

Conversation

craff
Copy link
Contributor

@craff craff commented May 23, 2023 •
edited by anmonteiro

TODO: are the fixes for output_char and input_int really necessary and sufficient ?

fixes #103

This was referenced May 23, 2023
Closed
Closed
Merged
anmonteiro
anmonteiro reviewed May 26, 2023
View reviewed changes
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable, but I wonder: should the exceptions we throw be more well defined? e.g. if I want to match on output_string failing, should I catch | Failure ...?

@craff
Copy link
Contributor Author

craff commented May 26, 2023 •
edited

Normally this exception should never be raised: writing/reading 0 bytes is not in principle possible without reporting an error from the lower level functions. We could replace by assert false ?

I added the exception to make sure a loop is not possible.

anmonteiro
anmonteiro reviewed May 28, 2023
View reviewed changes
src/ssl.ml Outdated Show resolved Hide resolved
anmonteiro
anmonteiro reviewed May 28, 2023
View reviewed changes
src/ssl.ml Outdated Show resolved Hide resolved
@anmonteiro anmonteiro merged commit caf08bd into savonet:master May 28, 2023
6 checks passed
@anmonteiro
Copy link
Collaborator

Thanks, I made a few tweaks to the error messages and added a changes entry.

@anmonteiro anmonteiro mentioned this pull request Jun 2, 2023
Closed
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Jun 2, 2023
@anmonteiro
[new release] ssl (0.6.0)
a337c59 
CHANGES:

- Raise an error when `Ssl.flush` isn't successful (savonet/ocaml-ssl#104, savonet/ocaml-ssl#120)
- Add an API-compatible `Ssl.Runtime_lock` module. The functions in this module
  don't release the OCaml runtime lock. While they don't allow other OCaml
  threads to run concurrently, they don't perform any copying in the underlying
  data, leading certain workloads to be faster than their counterparts that
  release the lock. (savonet/ocaml-ssl#106)
- Guarantee `Ssl.output_string` writes the whole string by retrying the
  operation with unwritten bytes (savonet/ocaml-ssl#103, savonet/ocaml-ssl#116)
- Fix calls in C stubs that need to call `ERR_clear_error` before the underlying
  OpenSSL call (savonet/ocaml-ssl#118)
- Add a module `Ssl.Error` to retrieve OpenSSL errors in a structured way (savonet/ocaml-ssl#119)
- Deprecate Ssl.{SSLv23,SSLv3,TLSv1,TLSv1_1}, which were were formally
  deprecated in March 2021 and earlier (savonet/ocaml-ssl#115).
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
@craff @anmonteiro
start work on noalloc (raising exception on ocaml side)
261baa4 
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
@craff @anmonteiro
start work on noalloc (raising exception on ocaml side)
4787e2c 
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
@craff @anmonteiro
start work on noalloc (raising exception on ocaml side)
da51501 
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit that referenced this pull request Jun 2, 2023
@craff @anmonteiro
start work on noalloc (raising exception on ocaml side)
872e154 
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue #103 for output_string, output_char and input_int (#116)

* fix issue #103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Jun 3, 2023
@anmonteiro
[new release] ssl (0.6.0)
512f902 
CHANGES:

- Raise an error when `Ssl.flush` isn't successful (savonet/ocaml-ssl#104, savonet/ocaml-ssl#120)
- Add an API-compatible `Ssl.Runtime_lock` module. The functions in this module
  don't release the OCaml runtime lock. While they don't allow other OCaml
  threads to run concurrently, they don't perform any copying in the underlying
  data, leading certain workloads to be faster than their counterparts that
  release the lock. (savonet/ocaml-ssl#106)
- Guarantee `Ssl.output_string` writes the whole string by retrying the
  operation with unwritten bytes (savonet/ocaml-ssl#103, savonet/ocaml-ssl#116)
- Fix calls in C stubs that need to call `ERR_clear_error` before the underlying
  OpenSSL call (savonet/ocaml-ssl#118)
- Add a module `Ssl.Error` to retrieve OpenSSL errors in a structured way (savonet/ocaml-ssl#119)
- Deprecate Ssl.{SSLv23,SSLv3,TLSv1,TLSv1_1}, which were were formally
  deprecated in March 2021 and earlier (savonet/ocaml-ssl#115).
@anmonteiro anmonteiro mentioned this pull request Jun 3, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

output_string should be a loop
2 participants
@craff @anmonteiro