config memory limits: handle values larger than (signed) LLONG_MAX by hwware · Pull Request #9313 · redis/redis

hwware · 2021-08-03T20:52:52Z

This PR is for #9071.

This aims to solve the issue in CONFIG SET maxmemory can only set maxmemory to up to 9223372036854775807 (2^63) while the maxmemory should be ULLONG. Added a memtoull function to convert a string representing an amount of memory into the number of bytes (similar to memtoll but for ull). Also added ull2string to convert a ULLong to string (Similar to ll2string).

madolson · 2021-08-04T01:01:16Z

Do we actually think this is worth implementing? This seems like a lot of effort to solve a problem that doesn't really matter. Thoughts?

oranagra · 2021-08-05T07:36:29Z

i agree this is not a real problem, and if we solve it is only in order to have a cleaner code.
in this case cleaner code is one that doesn't have unexpected truncation, but i'm not sure it worth it.
maybe changing the high limit of this config (and others) to LLONG_MAX rather than ULLONG_MAX is enough?

madolson · 2021-08-07T02:41:37Z

I agree with changing the upper limit to LLONG_MAX.

Super-long · 2021-08-07T07:27:58Z

I think this is not meaningful in terms of the problem to be solved. But I think it’s not a bad thing to provide the ability to configure ull, maybe it’s better to modify it when really needed.

oranagra

@hwware i looked at it again.
I agree it would be nice to solve the problem and have cleaner code with no unexpected truncation, but i really don't like the code duplication (code is not cleaner this way!).

I think there's a good chance you can do better.

one example is that ull2string and ll2string can probably be unified in some way, maybe by putting most logic in ull2string and have ll2string call it and to the pre/post fixups.

regarding memtoull and memtoll there's probably no need for both, memory configs are always 0 (maybe with the exception of what i'm attempting in #7911).

I also imagine that the big if-else blocks with 90% common code you added in numericConfigSet and numericConfigGet can in some way be written to share code.

hwware · 2021-08-09T13:24:55Z

Ok got it. Thanks for the review, I will refactor and adjust the code.

src/config.c

src/util.c

oranagra · 2021-08-17T20:20:53Z

looks good. (cleaner than the initial version).
i still think the negative argument needs to be removed from ull2string.
after that i'm good to merge this.

hwware · 2021-08-20T14:11:20Z

@oranagra , I pushed the changes addressing the negative argument. Please take a look. Thanks

and move comment to the right place

oranagra · 2021-08-20T15:33:21Z

@hwware please look at my edit to see what i meant.
now ull2string doesn't need to have anything to do with negative numbers (it's all handled in ll2string)
it came out a bit more complicated that i thought it would since i also handled error case of insufficient buffer size.
also moved the comment about the algorithm to the right place.
please let me know if you see any problems.

hwware · 2021-08-20T16:15:37Z

@oranagra yes this looks fine to me too.

src/util.c

Co-authored-by: Binbin <[email protected]>

oranagra · 2021-08-22T05:56:26Z

@hwware please take a look at my changes.
it fixes config set to use the right variable type (unsigned long long), but also change the timeout validation to use the right parsing function).
i suppose this makes it a breaking change (in case someone used 10k for timeout instead of using 10000. although i think that person would have got a timeout of 10, right?

enjoy-binbin · 2021-08-22T06:09:39Z

The last change is a bit similar to this one #9308

oranagra · 2021-08-22T10:34:44Z

@enjoy-binbin right, i had to do part of it because of the variable was the wrong type and there was a negative check, and then i noticed that the timeout was verified with the wrong method.
anyway, your PR goes deeper (validation of config file parsing), and will need to be rebased anyway after this one is merged, so let's do that after.

hwware · 2021-08-23T16:54:57Z

@oranagra I think the changes are good. And yes you are right, in the case the use specifies 10k as timeout instead of 10000 then the timeout will be set to 10.

This one follow redis#9313 and goes deeper (validation of config file parsing) Move the check/update logic to a new updateClientOutputBufferLimit function. So that it can be used in CONFIG SET and config file parsing.

This one follow #9313 and goes deeper (validation of config file parsing) Move the check/update logic to a new updateClientOutputBufferLimit function. So that it can be used in CONFIG SET and config file parsing.

…edis#9313) This aims to solve the issue in CONFIG SET maxmemory can only set maxmemory to up to 9223372036854775807 (2^63) while the maxmemory should be ULLONG. Added a memtoull function to convert a string representing an amount of memory into the number of bytes (similar to memtoll but for ull). Also added ull2string to convert a ULLong to string (Similar to ll2string).

This one follow redis#9313 and goes deeper (validation of config file parsing) Move the check/update logic to a new updateClientOutputBufferLimit function. So that it can be used in CONFIG SET and config file parsing.

…edis#9313) This aims to solve the issue in CONFIG SET maxmemory can only set maxmemory to up to 9223372036854775807 (2^63) while the maxmemory should be ULLONG. Added a memtoull function to convert a string representing an amount of memory into the number of bytes (similar to memtoll but for ull). Also added ull2string to convert a ULLong to string (Similar to ll2string).

hwware added 12 commits July 30, 2021 14:49

Fix for maxmemory bug

f4928b4

Update util.c

f2ad5e5

update

c5f0396

Update for ulltostring

3e266c6

Update config.c

70127db

Update for memtoull

6cbf4aa

Update config.c

95267d5

Update config.c

70e7d7e

Update config.c

fa065fc

Update config.c

f9405bf

Update config.c

16e467c

Update config.c

f48d42e

Update config.c

de76ab0

hwware mentioned this pull request Aug 4, 2021

[BUG] The value exceeding LLONG_MAX in createULongLongConfig is invalid #9071

Closed

hwware marked this pull request as ready for review August 4, 2021 17:01

hwware mentioned this pull request Aug 4, 2021

The value exceeding LLONG_MAX in createULongLongConfig is invalid hwware/redis#10

Open

oranagra linked an issue Aug 5, 2021 that may be closed by this pull request

[BUG] The value exceeding LLONG_MAX in createULongLongConfig is invalid #9071

Closed

oranagra reviewed Aug 8, 2021

View reviewed changes

hwware marked this pull request as draft August 11, 2021 21:42

hwware added 2 commits August 11, 2021 17:55

Update for code refactoring

0cffd69

Update util.c

b618dc7

oranagra reviewed Aug 12, 2021

View reviewed changes

src/config.c Outdated Show resolved Hide resolved

src/util.c Outdated Show resolved Hide resolved

oranagra changed the title ~~Issue 9071 maxmemory~~ config memory limits: handle values larger than (signed) LLONG_MAX Aug 12, 2021

hwware added 2 commits August 16, 2021 13:46

Update

4aa7f3f

Update util.c

a217f4d

reverting some unintended changes

998e3b5

Changing the negative argument in ull2string

e399d0d

remove all negative numbers related code from ull2string

6fd1d85

and move comment to the right place

enjoy-binbin reviewed Aug 21, 2021

View reviewed changes

src/util.c Outdated Show resolved Hide resolved

oranagra and others added 4 commits August 22, 2021 08:39

Update src/util.c

79af7bf

Co-authored-by: Binbin <[email protected]>

fix error checking in config set

81f7103

typo

362d2be

i should stop being lazy, and stop editing code in GH web UI..

4021f22

i really should stop being that lazy!

6bab9d1

last one.

e078008

oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Aug 22, 2021

oranagra approved these changes Aug 23, 2021

View reviewed changes

oranagra merged commit 641780a into redis:unstable Aug 23, 2021

enjoy-binbin mentioned this pull request Aug 24, 2021

Better error handling for updateClientOutputBufferLimit. #9308

Merged

Conversation

hwware commented Aug 3, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Aug 4, 2021

Uh oh!

oranagra commented Aug 5, 2021

Uh oh!

madolson commented Aug 7, 2021

Uh oh!

Super-long commented Aug 7, 2021

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

hwware commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Aug 17, 2021

Uh oh!

hwware commented Aug 20, 2021

Uh oh!

oranagra commented Aug 20, 2021

Uh oh!

hwware commented Aug 20, 2021

Uh oh!

Uh oh!

oranagra commented Aug 22, 2021

Uh oh!

enjoy-binbin commented Aug 22, 2021

Uh oh!

oranagra commented Aug 22, 2021

Uh oh!

hwware commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hwware commented Aug 3, 2021 •

edited by oranagra

Loading

hwware commented Aug 9, 2021 •

edited

Loading