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

Deprecate EVP_MD_CTX_{set_}update_fn() #14008

Closed
wants to merge 1 commit into from
Closed

Deprecate EVP_MD_CTX_{set_}update_fn() #14008

wants to merge 1 commit into from

Conversation

richsalz
Copy link
Contributor

They are still used internally in legacy code.

Also fixed up some minor things in EVP_DigestInit.pod

Fixes: #14003

@t8m
Copy link
Member

t8m commented Feb 1, 2021

Could you please add CHANGES.md entry?

@t8m t8m added the branch: master Merge to master branch label Feb 1, 2021
@t8m t8m added this to the 3.0.0 beta1 milestone Feb 1, 2021
@richsalz
Copy link
Contributor Author

richsalz commented Feb 1, 2021

Added CHANGES.md, updated the commit, rebased, pushed.

CHANGES.md Outdated
@@ -29,6 +29,11 @@ OpenSSL 3.0

*Tomas Mraz*

* Deprecate EVP_MD_CTX_set_update_fn() and EVP_MD_CTX_update_fn()
although they are still used internally in legacy (engine) code.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the although they are still used internally in legacy (engine) code. is really bringing any important information to the user. I think something like as they are not useful with non-deprecated functionality. would be better. Or something in that sense.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry for being picky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought what I had gave more info, but I don't care very much. Wording changed and updated commit pushed.

They are still used internally in legacy code.

Also fixed up some minor things in EVP_DigestInit.pod

Fixes: #14003
@t8m t8m added the approval: review pending This pull request needs review by a committer label Feb 1, 2021
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

LGTM

@paulidale paulidale 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 Feb 2, 2021
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.

Yes!

(I have some experimental code that drops the internal use too)

@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Feb 3, 2021
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Feb 3, 2021
openssl-machine pushed a commit that referenced this pull request Feb 3, 2021
They are still used internally in legacy code.

Also fixed up some minor things in EVP_DigestInit.pod

Fixes: #14003

Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #14008)
@t8m
Copy link
Member

t8m commented Feb 3, 2021

Merged to master. Thank you for your contribution!

@t8m t8m closed this Feb 3, 2021
@richsalz richsalz deleted the fix-14003 branch February 3, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVP_MD_CTX_update_fn and EVP_MD_CTX_set_update_fn should be deprecated
6 participants