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 the define for two files in the key rotation #188

Open
wants to merge 1 commit into
base: TDE_REL_17_STABLE
Choose a base branch
from
Open

Remove the define for two files in the key rotation #188

wants to merge 1 commit into from

Conversation

jeltz
Copy link
Collaborator

@jeltz jeltz commented Apr 4, 2025

Since a rotation only involves two keys, old and new, this define does not make anything clearer.

@jeltz jeltz requested review from dutow and dAdAbird as code owners April 4, 2025 12:29
AndersAstrand
AndersAstrand approved these changes Apr 4, 2025
View reviewed changes
Copy link

@AndersAstrand AndersAstrand left a comment

Choose a reason for hiding this comment

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

I think this is a good quick drive-by cleanup, but I wonder if using separate variables instead of an array of 2 and get rid of all the defines is even cleaner.

new_key_curr_pos isn't uglier than curr_pos[NEW_PRINCIPAL_KEY] I mean.

@jeltz
Use separate variables instead of an array when rotating files
e8a845e 
This makes the code easier to read plus gives us things like letting
the compiler check for unused variables.
@jeltz jeltz force-pushed the tde/rm-pointless-define branch from ce362d1 to e8a845e Compare April 4, 2025 16:00
@jeltz
Copy link
Collaborator Author

jeltz commented Apr 4, 2025

Good, suggestion. I rewrote the patch to do that and I would say the new version is nicer.

@AndersAstrand
Copy link

The initialization of curr_pos that was there before but not now seems to have been completely unnecessary as far as I can see. So this new version also looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndersAstrand AndersAstrand AndersAstrand approved these changes

@dutow dutow Awaiting requested review from dutow dutow is a code owner

@dAdAbird dAdAbird Awaiting requested review from dAdAbird dAdAbird is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jeltz @AndersAstrand