Skip to content

Fixes #24322#25665

Closed
christriants wants to merge 1 commit into
openssl:masterfrom
christriants:openssl-24322
Closed

Fixes #24322#25665
christriants wants to merge 1 commit into
openssl:masterfrom
christriants:openssl-24322

Conversation

@christriants
Copy link
Copy Markdown
Contributor

@christriants christriants commented Oct 10, 2024

Fixes #24322

This pull request updates documentation for SSL_SESSION_set_time_ex.

Checklist
  • documentation is added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Oct 10, 2024
@christriants
Fixes openssl#24322
2dec974 
CLA: trivial
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Oct 10, 2024
slontis
slontis reviewed Oct 10, 2024
View reviewed changes
Comment thread doc/man3/SSL_SESSION_get_time.pod

SSL_SESSION_set_timeout() returns 1 on success.

If any of the function is passed the NULL pointer for the session B<s>,
Copy link
Copy Markdown
Member

@slontis slontis Oct 10, 2024

Choose a reason for hiding this comment

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

Suggested change
If any of the function is passed the NULL pointer for the session B<s>,
All functions return 0 if the session B<s> is NULL.

Copy link
Copy Markdown
Contributor

@tom-cosgrove-arm tom-cosgrove-arm Oct 11, 2024

Choose a reason for hiding this comment

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

This is pre-existing. It looks like there's a lot of work to get this whole file sorted, shall we just take the trivial changes here and worry about the rest later?

mattcaswell
mattcaswell reviewed Oct 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

The below comment is a suggestion (but I would approve anyway if this is not done - once the comment from @slontis is responded to)

Comment thread doc/man3/SSL_SESSION_get_time.pod
SSL_SESSION_set_time_ex() returns time on success.

SSL_SESSION_set_timeout() returns 1 on success.

Copy link
Copy Markdown
Member

@mattcaswell mattcaswell Oct 11, 2024

Choose a reason for hiding this comment

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

While about it could we also document the return values for the other functions in the synopsis not mentioned here? e.g. SSL_get_timeout/SSL_set_timeout/SSL_SESSION_get_time/SSL_SESSION_set_time/SSL_get_time/SSL_set_time

@tom-cosgrove-arm
Copy link
Copy Markdown
Contributor

tom-cosgrove-arm commented Oct 11, 2024

I am happy with the changes as-made being under CLA: trivial, but not sure that adding missing information would continue to be trivial

@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Oct 11, 2024

I am happy with the changes as-made being under CLA: trivial, but not sure that adding missing information would continue to be trivial

Ah - right. Good point. I had not spotted this was CLA: trivial.

@mattcaswell mattcaswell added cla: trivial One of the commits is marked as 'CLA: trivial' branch: master Applies to master branch approval: review pending This pull request needs review by a committer triaged: documentation The issue/pr deals with documentation (errors) tests: exempted The PR is exempt from requirements for testing labels Oct 11, 2024
@t8m t8m added the branch: 3.4 Applies to openssl-3.4 label Oct 11, 2024
tom-cosgrove-arm
tom-cosgrove-arm approved these changes Oct 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

ok with trivial

mattcaswell
mattcaswell approved these changes Oct 11, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Agree trivial

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 11, 2024
@christriants
Copy link
Copy Markdown
Contributor Author

christriants commented Oct 11, 2024

Thank you, all. Would you mind adding the label hacktoberfest-accepted for this PR, as a mini contribution for https://hacktoberfest.com/ ? Thanks!

@mattcaswell mattcaswell added the hacktoberfest-accepted Accepted for hacktoberfest, will be merged. label Oct 11, 2024
@mattcaswell
Copy link
Copy Markdown
Member

mattcaswell commented Oct 11, 2024

Thank you, all. Would you mind adding the label hacktoberfest-accepted for this PR, as a mini contribution for https://hacktoberfest.com/ ? Thanks!

Done.

@openssl-machine
Copy link
Copy Markdown
Collaborator

openssl-machine commented Oct 12, 2024

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@tom-cosgrove-arm tom-cosgrove-arm added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 12, 2024
shahsb
shahsb approved these changes Oct 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@shahsb shahsb left a comment

Choose a reason for hiding this comment

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

Okay with CLA: trivial.

LGTM. Thanks for making the changes!

@t8m
Copy link
Copy Markdown
Member

t8m commented Oct 14, 2024

Merged to the master and 3.4 branches after improving the commit message. Thank you for your contribution.

@t8m t8m closed this Oct 14, 2024
openssl-machine pushed a commit that referenced this pull request Oct 14, 2024
@christriants @t8m
Update documentation for SSL_SESSION_set_time_ex()
f1607c8 
Fixes #24322

CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #25665)
openssl-machine pushed a commit that referenced this pull request Oct 14, 2024
@christriants @t8m
Update documentation for SSL_SESSION_set_time_ex()
3a04840 
Fixes #24322

CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from #25665)

(cherry picked from commit f1607c8)
coolshrid pushed a commit to coolshrid/openssl that referenced this pull request Nov 9, 2024
@christriants @coolshrid
Update documentation for SSL_SESSION_set_time_ex()
9695489 
Fixes openssl#24322

CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from openssl#25665)
@xnox xnox mentioned this pull request Nov 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slontis slontis slontis left review comments

@mattcaswell mattcaswell mattcaswell approved these changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

+1 more reviewer

@shahsb shahsb shahsb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.4 Applies to openssl-3.4 cla: trivial One of the commits is marked as 'CLA: trivial' hacktoberfest-accepted Accepted for hacktoberfest, will be merged. tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SSL_SESSION_set_time incorrect documentation

7 participants

@christriants @tom-cosgrove-arm @mattcaswell @openssl-machine @t8m @shahsb @slontis