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

Replace the 'SSL' broken link in ciphers doc with one pointing to security levels explained #1898

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 11, 2016

SSL_CTX_set_security_level() seems not being referenced from elsewhere and an example using SECLEVEL is provided just above the 'SEE ALSO' section

@@ -717,7 +717,7 @@ Set security level to 2 and display all ciphers consistent with level 2:

=head1 SEE ALSO

L<s_client(1)>, L<s_server(1)>, L<ssl(3)>
Copy link
Member

Choose a reason for hiding this comment

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

Section 3 is wrong, it's in section 7 (have a look in doc/man7)

I think I would prefer if the section number was changed unless there's a strong reason to change the reference.

Copy link
Author

Choose a reason for hiding this comment

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

You'r right. Sorry for missing that. May I still propose to add the link to 'security levels' which I found very usefull ?
PR changed.

Copy link
Author

Choose a reason for hiding this comment

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

In such a case, is it better to modify the PR title ?

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think ciphers is the place for the security-levels link. it should be in man7/ssl.

Copy link
Author

@ghost ghost Nov 11, 2016

Choose a reason for hiding this comment

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

OK, I am going to update the PR.
Done.

Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

Looks good. Btw, this inspired me to look into manual links, and boy is there more. See #1900

@richsalz richsalz self-assigned this Nov 13, 2016
@richsalz richsalz added the approval: done This pull request has the required number of approvals label Nov 13, 2016
levitte pushed a commit that referenced this pull request Nov 13, 2016
…eems not being referenced from elsewhere

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1898)
levitte pushed a commit that referenced this pull request Nov 13, 2016
…eems not being referenced from elsewhere

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #1898)
(cherry picked from commit e330f55)
@richsalz
Copy link
Contributor

e330f55 on master; 66bf3bc on 1.1.0 thanks!

@richsalz richsalz closed this Nov 13, 2016
@ghost ghost deleted the docs branch November 13, 2016 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants