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

docs: Documentation clean up #3329

Merged
merged 1 commit into from
Jun 8, 2022
Merged

Conversation

lundinc2
Copy link
Contributor

@lundinc2 lundinc2 commented May 24, 2022

Resolved issues:

resolves 3314

Description of changes:

  • Add a README section that points to Doxygen API docs.
  • Add a README section on how to use Doyxgen.
  • Add some missed documentation to s2n.h.
  • Removed code snippets from s2n.h.
  • Removed basic API definitions from the usage guide

Call-outs:

Address any potentially confusing code. Is there code added that needs to be cleaned up later? Is there code that is missing because it’s still in development?

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Also talked offline: I'm not sure about putting examples, especially large ones or ones involving multiple APIs, in the Doxygen documentation. I also worry that we've lost some of the actually useful "how APIs interact" documentation from the Usage guide.

@lundinc2 lundinc2 force-pushed the documentation-readme branch 2 times, most recently from 7d96994 to 4e0aa5e Compare May 25, 2022 20:00
@lundinc2 lundinc2 requested review from lrstewart and maddeleine May 25, 2022 21:35
Copy link
Contributor

@maddeleine maddeleine left a comment

Choose a reason for hiding this comment

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

I don't think this PR actually resolves #3314, it doesn't include the CSS changes.

@lundinc2
Copy link
Contributor Author

I don't think this PR actually resolves #3314, it doesn't include the CSS changes.

I'll do a separate PR to introduce CSS changes

@lundinc2 lundinc2 requested a review from maddeleine May 25, 2022 22:53
@dougch dougch added the s2n-core team label May 26, 2022
@maddeleine
Copy link
Contributor

I don't think this PR actually resolves #3314, it doesn't include the CSS changes.

I'll do a separate PR to introduce CSS changes

Yeah I just meant like, this PR will resolve #3314, which we don't want, because the issue hasn't been resolved yet.

api/s2n.h Outdated
Comment on lines 1742 to 1730
* @note Unlike OpenSSL, repeated calls to s2n_send() should not duplicate the original parameters, but should
* update `buf` and `size` per the indication of size written. For example;
* ```c
Copy link
Contributor

@lrstewart lrstewart May 26, 2022

Choose a reason for hiding this comment

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

I think I was wrong about removing examples. This one seems pretty integral to this @note.

Same for s2n_sendv_with_offset and s2n_recv.

api/s2n.h Outdated
Comment on lines 2724 to 2729
*
* @warning The string "TLS_NULL_WITH_NULL_NULL" is returned before the TLS handshake has been performed.
* This does not mean that the ciphersuite "TLS_NULL_WITH_NULL_NULL" will be used by the connection,
* it is merely being used as a placeholder.
*
* @note This function is only accurate after the TLS handshake.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed? Didn't another PR just add these notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an error on my part. I'll revert this change.

@lundinc2 lundinc2 force-pushed the documentation-readme branch 2 times, most recently from 219dd6a to 21bd600 Compare June 7, 2022 15:17
@lundinc2 lundinc2 force-pushed the documentation-readme branch from 21bd600 to b21b9a9 Compare June 7, 2022 15:47
@lundinc2 lundinc2 requested a review from lrstewart June 7, 2022 15:50
@lundinc2 lundinc2 enabled auto-merge (squash) June 7, 2022 17:20

**s2n_config_new** returns a new configuration object suitable for associating certs and keys.
This object can (and should) be associated with many connection objects.
Setting the `S2N_SELF_SERVICE_BLINDING` option with `s2n_connection_set_blinding()` turns off this behavior. This is useful for applications that are handling many connections in a single thread. In that case, if `s2n_recv()` or `s2n_negotiate()` return an error, self-service applications must call `s2n_connection_get_delay()` and pause activity on the connection for the specified number of nanoseconds before calling `close()` or `shutdown()`. `s2n_shutdown()` will fail if called before the blinding delay elapses.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: double space

Suggested change
Setting the `S2N_SELF_SERVICE_BLINDING` option with `s2n_connection_set_blinding()` turns off this behavior. This is useful for applications that are handling many connections in a single thread. In that case, if `s2n_recv()` or `s2n_negotiate()` return an error, self-service applications must call `s2n_connection_get_delay()` and pause activity on the connection for the specified number of nanoseconds before calling `close()` or `shutdown()`. `s2n_shutdown()` will fail if called before the blinding delay elapses.
Setting the `S2N_SELF_SERVICE_BLINDING` option with `s2n_connection_set_blinding()` turns off this behavior. This is useful for applications that are handling many connections in a single thread. In that case, if `s2n_recv()` or `s2n_negotiate()` return an error, self-service applications must call `s2n_connection_get_delay()` and pause activity on the connection for the specified number of nanoseconds before calling `close()` or `shutdown()`. `s2n_shutdown()` will fail if called before the blinding delay elapses.

Comment on lines +318 to +319
### Stacktraces
s2n-tls has an mechanism to capture stacktraces when errors occur.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
### Stacktraces
s2n-tls has an mechanism to capture stacktraces when errors occur.
### Stacktraces
s2n-tls has a mechanism to capture stacktraces when errors occur.

@lundinc2 lundinc2 merged commit af79699 into aws:main Jun 8, 2022
lundinc2 added a commit to lundinc2/s2n-tls that referenced this pull request Jun 9, 2022
Re-write a portion of the USAGE GUIDE to move it towards being a guide vs an API reference.
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Jun 27, 2022
Re-write a portion of the USAGE GUIDE to move it towards being a guide vs an API reference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants