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

fix(pdk) get_phases_names wrong lshift #8208

Merged
merged 1 commit into from
Dec 17, 2021
Merged

fix(pdk) get_phases_names wrong lshift #8208

merged 1 commit into from
Dec 17, 2021

Conversation

chronolaw
Copy link
Contributor

Summary

PHASES.n means lshift times, but now it equals to 13, it is 0x2000,
but the max phases admin_api is 0x10000000,
so the function get_phases_names can not get the right name.

I change the PHASES.n to 30, then we can support the all phases bits.

Full changelog

  • change PHASES.n to 30, means 2^30 = 0x40000000

Copy link
Member

@kikito kikito left a comment

Choose a reason for hiding this comment

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

You fix an existing problem (that we sometimes don't get the appropriate phase name for some phases) and I think that the change made here was as minimal as it could, which is usually good. So I am inclined to approving this.

@kikito
Copy link
Member

kikito commented Dec 17, 2021

(I also happen to think that the current implementation of kong/pdk/private/phases.lua is too clever and confusing - mostly because of two reasons: using a single PHASES table instead of two (one for number => name and a different one for name => number) and naming several things n (number of bits and bit being iterated over).

@kikito kikito merged commit 73368d5 into Kong:master Dec 17, 2021
@chronolaw chronolaw deleted the fix/pdk_check_phases branch December 17, 2021 23:49
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.

None yet

2 participants