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

Remove unused internal functions evp_pkey_ctx_get1_id_prov and evp_pkey_ctx_get1_id_len_prov #21329

Closed
wants to merge 3 commits into from
Closed

Remove unused internal functions evp_pkey_ctx_get1_id_prov and evp_pkey_ctx_get1_id_len_prov #21329

wants to merge 3 commits into from

Conversation

nv-dmd
Copy link

@nv-dmd nv-dmd commented Jun 30, 2023

Fixes #20701
Remove unused internal functions evp_pkey_ctx_get1_id_prov and evp_pkey_ctx_get1_id_len_prov

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 30, 2023
@nv-dmd nv-dmd changed the title Fixes #20701 Remove unused internal functions evp_pkey_ctx_get1_id_prov and evp_pkey_ctx_get1_id_len_prov Remove unused internal functions evp_pkey_ctx_get1_id_prov and evp_pkey_ctx_get1_id_len_prov Jun 30, 2023
@paulidale paulidale added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Jun 30, 2023
@t8m
Copy link
Member

t8m commented Jun 30, 2023

CI is relevant.

@t8m
Copy link
Member

t8m commented Jun 30, 2023

As this is just removal it would be acceptable with CLA: trivial. Could you please amend the commit message with git commit --amend so it contains CLA: trivial on a separate line in the commit message body?

@t8m t8m added the triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly label Jun 30, 2023
@slontis
Copy link
Member

slontis commented Jul 1, 2023

@levitte These look like leftovers from this..
86df26b
Not sure evp_pkey_ctx_set1_id_prov() is useful either?

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jul 2, 2023
@nv-dmd
Copy link
Author

nv-dmd commented Jul 3, 2023

Maybe I should also remove get1_id_data function, which is not used after the changes and so failed checks?

@nv-dmd
Copy link
Author

nv-dmd commented Jul 3, 2023

And can I do the same Pull Request in the 3.0 branch, because I use this branch? And what about 3.1 branch?

@t8m
Copy link
Member

t8m commented Jul 3, 2023

Maybe I should also remove get1_id_data function, which is not used after the changes and so failed checks?

Yes, please.

@t8m
Copy link
Member

t8m commented Jul 3, 2023

And can I do the same Pull Request in the 3.0 branch, because I use this branch? And what about 3.1 branch?

This will not be merged to 3.0 branch as this is just a cleanup and not a bug fix.

@nv-dmd
Copy link
Author

nv-dmd commented Jul 4, 2023

Is there anything else I need to do?

@t8m t8m added cla: trivial One of the commits is marked as 'CLA: trivial' and removed hold: cla required The contributor needs to submit a license agreement labels Jul 4, 2023
t8m
t8m approved these changes Jul 4, 2023
View reviewed changes
Copy link
Member

@t8m t8m 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 CLA: trivial.

@t8m
Copy link
Member

t8m commented Jul 4, 2023

We will squash the commits when merging.

@t8m t8m added the tests: exempted The PR is exempt from requirements for testing label Jul 4, 2023
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jul 4, 2023
paulidale
paulidale approved these changes Jul 4, 2023
View reviewed changes
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.

yes to trivial

@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 Jul 4, 2023
@slontis
Copy link
Member

slontis commented Jul 4, 2023

Is evp_pkey_ctx_set1_id_prov() used anywhere?

@nv-dmd
Copy link
Author

nv-dmd commented Jul 5, 2023

Is evp_pkey_ctx_set1_id_prov() used anywhere?

I agree with this. Function evp_pkey_ctx_set1_id_prov() is not used now and there is function EVP_PKEY_CTX_set1_id instead

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jul 5, 2023
t8m
t8m approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Still OK with CLA: trivial and assuming @paulidale is too.

@t8m t8m removed the hold: cla required The contributor needs to submit a license agreement label Jul 5, 2023
@openssl-machine
Copy link
Collaborator

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.

@nv-dmd
Copy link
Author

nv-dmd commented Jul 6, 2023

What else can I do?

@paulidale
Copy link
Contributor

Merged to master, thanks for the contribution.

@paulidale paulidale closed this Jul 7, 2023
openssl-machine pushed a commit that referenced this pull request Jul 7, 2023
@paulidale
Remove unused internal functions
926601d 
CLA: trivial

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #21329)
@nv-dmd nv-dmd deleted the issue_#20701 branch July 7, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch cla: trivial One of the commits is marked as 'CLA: trivial' tests: exempted The PR is exempt from requirements for testing triaged: cleanup The issue/pr deals with cleanup of comments/docs not altering code significantly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash if ctx equal to NULL is passed to evp_pkey_ctx_get1_id_prov or evp_pkey_ctx_get1_id_len_prov
5 participants
@nv-dmd @t8m @slontis @openssl-machine @paulidale