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

Update hiredis sds with improvements found in redis #1045

Merged
merged 3 commits into from Apr 5, 2022

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Feb 1, 2022

Update hiredis sds with improvements found in redis for convergence,
like the overflow mentioned in redis/redis#9706

Handpicked from:
redis/redis#4568
redis/redis#9584
redis/redis#8522

@bjosv
Copy link
Contributor Author

bjosv commented Feb 1, 2022

Ouch, it looks like CentOS8 has gone EOL and that we need to do some updates for CI.

@michael-grunder
Copy link
Collaborator

I actually ran into the CentOS issue yesterday and even complained about it on Twitter 😄

I believe there is a short-term solution to this. I'll get that up now.

@bjosv
Copy link
Contributor Author

bjosv commented Feb 1, 2022

😀 ahh, I missed that one in my feed.
Yeah, at least they didn't kill it at New Year's Eve when it hit EOL... so they had it prolonged to the Chinese New Year instead 🙄

Equivalent changes introduced to redis sds.c via:
redis/redis#4568
Equivalent changes introduced to redis sds.c via:
redis/redis#8522
redis/redis#9584
@michael-grunder
Copy link
Collaborator

Thanks for porting these over.

I wonder if we can avoid the asserts on overflow though. Simply return NULL instead so the error is just treated like an OOM for clients of the library.

@bjosv
Copy link
Contributor Author

bjosv commented Feb 2, 2022

I wonder if we can avoid the asserts on overflow though. Simply return NULL instead so the error is just treated like an OOM for clients of the library.

Yes, you are right, much better. This follows how its handled in other places

@bjosv bjosv closed this Feb 2, 2022
@bjosv bjosv reopened this Feb 2, 2022
@bjosv
Copy link
Contributor Author

bjosv commented Feb 2, 2022

Sorry, touchpad gone wild.. updated the PR with the change.

@michael-grunder michael-grunder merged commit 7c44a9d into redis:master Apr 5, 2022
@michael-grunder
Copy link
Collaborator

Merged, thanks!

@bjosv bjosv deleted the sds-updates branch April 6, 2022 06:26
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