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

Porting over some ICP code that was missed in #4760 #5251

Merged
merged 1 commit into from
Oct 10, 2016

Conversation

tcaputi
Copy link
Contributor

@tcaputi tcaputi commented Oct 9, 2016

When #4760 was merged tests were added to ensure that the new
checksums were working preperly. However, some of the functionality
for sha2 functions were not ported over, resulting in some Coverity
defects and code that would be unstable when needed in the future.
This patch simply ports over the missing code and fixes the defects in
the process.

When openzfs#4760 was merged tests were added to ensure that the new checksums
were working preperly. However, some of the functionality for sha2
functions were not ported over, resulting in some Coverity defects and
code that would be unstable when needed in the future. This patch
simply ports over the missing code and fixes the defects in the
process.
@mention-bot
Copy link

@tcaputi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @luozhengzheng and @tonyhutter to be potential reviewers.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This looks good and brings our version of sha2_mod.c close to in sync with upstream. The major differences which can't, and shouldn't, be avoided are related to module init/registration, fini/unregistration, and various fixes for gcc warnings.

The only exception I see is that CRYPTO_DATA_MBLK functionality wasn't added back. But you've already clearly explained why that's the case in illumos-crypto.c so that's fine. If thanks something we ever need in the future it can be added back in then. In the meanwhile it's less code which needs to be maintained which is good.

Let me know if you're happy with this as the final version of this PR and I'll merge it.

@tcaputi
Copy link
Contributor Author

tcaputi commented Oct 10, 2016

@behlendorf I'm fine with it as is, but if you would like I can also add in the memory leak fixes for the destructors that we didn't add to the ICP. I'll try to get to that before the end of the day.

@behlendorf
Copy link
Contributor

@tcaputi let me merge this change right now as is then since I think it stands alone nicely. Then you can open a new PR with the memory leak fixes.

@tcaputi
Copy link
Contributor Author

tcaputi commented Oct 10, 2016

Sounds good to me

@behlendorf behlendorf merged commit 57f1660 into openzfs:master Oct 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants