util: ParseByteUnits - Parse a string with suffix unit by dougEfresh · Pull Request #23249 · bitcoin/bitcoin

dougEfresh · 2021-10-11T10:25:44Z

A convenience utility for parsing human readable strings sizes e.g. 500G is 500 * 1 << 30

The argument/setting maxuploadtarget now accept human readable byte units [k|K|m|M|g|G||t|T]
This change backward compatible, defaults to M if no unit specified.

src/init.cpp

src/util/strencodings.cpp

src/util/strencodings.h

src/util/strencodings.cpp

dougEfresh · 2021-10-13T14:58:42Z

Updated with

Replacing size_t in favor of uint64_t (windows/mac didn't play nice with size_t)
Adding InitError on parsing failure
Simplified case statement in ParseByteUnit
1000 for lowercase 1024 base for UPPERCASE

What I need help with is overflows. I use __builtin_mul_overflow but there must be a better/easier solution.

DrahtBot · 2021-10-14T20:37:18Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22834 (net: respect -onlynet= when making outbound connections by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/util/strencodings.cpp

promag

Concept ACK. However, I think the implementation can be improved. Why not check the last char first? If it's not a digit then extract it and validate to get the multiplier. Only then parse the number.

src/util/strencodings.cpp

src/init.cpp

ryanofsky

Code review and lightly tested ACK c5d4a59cf91f54a2027851b32cf83cfcb04cd210. I left some suggestions below, but none are important so feel free to ignore them.

It might be good to add a release note in the doc/ directory mentioning the new argument verification, since in theory it could make a configuration that used to work previously now cause an error on startup.

It would also be good to mention new behavior in the PR description, since current description is a little ambiguous about whether this is only adding a utility function or changing behavior.

src/init.cpp

src/util/strencodings.cpp

src/test/util_tests.cpp

dougEfresh · 2021-11-02T22:15:57Z

It might be good to add a release note in the doc/ directory mentioning the new argument verification, since in theory it could make a configuration that used to work previously now cause an error on startup.

Added to release docs

It would also be good to mention new behavior in the PR description, since current description is a little ambiguous about whether this is only adding a utility function or changing behavior.

Updated PR description.

Thanks for feedback @ryanofsky

ryanofsky

Code review ACK f1f8a772b0daf8cc0c40f0fc54e82618ad242ddd. Just suggested changes since last review (thanks!)

src/init.cpp

src/test/util_tests.cpp

src/util/strencodings.cpp

src/init.cpp

vasild

ACK f3f88062f4cdb0cc2d1af221a5a9da9ee2ea0e46

Some styling nits below, which would be good to address before merge.

The style rules are defined in src/.clang-format. You can use contrib/devtools/clang-format-diff.py to properly format the patch.

src/util/strencodings.h

src/util/strencodings.cpp

src/init.cpp

vasild

ACK 6ebb47626ee30576548e8b3c01ad961761e28562

dougEfresh · 2021-11-09T16:49:48Z

@MarcoFalke would you mind reviewing? Thanks

vasild

ACK 4451a0000dda5daba4b1624dce1c95f1914e7251

promag

Code review ACK 4451a0000dda5daba4b1624dce1c95f1914e7251.

src/util/strencodings.cpp

maflcko · 2021-11-10T09:24:33Z

src/util/strencodings.cpp

With this breaking change (no longer ignoring whitespace), which I think is fine, it might be best to also reject sign characters (+-) and use ToIntegral<uint64_t>.

I have tests for +-, whitespace, invalid unit...etc
I'm now using ToIntegral.
I've added an assert(default_multiplier > 0) but I don't know how to test this. Any ideas examples?

You cannot have the test trigger an assert() and continue successfully. Instead, you can throw std::out_of_range from the function and try/catch it from the test (BOOST_CHECK_THROW()). But in this case maybe it is best/simpler to just remove the parameter. Then there is no need to test edge cases for it :)

maflcko · 2021-11-10T09:26:51Z

src/util/strencodings.h

Should the default multiplier have a default? It seems like the default is something that is per-arg and not project-wide.

Agreed. I will remove the default.

How about 1 as a default multiplier ?

How about removing the argument? It's not needed. Either way please update doc comment above.

default of 1 seems natural, so my latest change defaults to 1

How about removing the argument? It's not needed.

Indeed! I would remove that unused code. No parameter, no problem :) (what should be its default value? should it have default value? how to test 0 if we have assert()?)

i do need this multiplier argument. the intent of this function is to be used with maxupload, dbcache...etc. they inheritly have a default multiplier (mostly MiB). the main purpose is maintain backwards capability, e. g. maxupload=300 becomes 300m.
As I write this, i think I will change the argument to a an enum with the valid units [k|K|m|M...]
with no default value. This makes it clear and easy to verify/test.

@vasild @MarcoFalke @promag
I added an enum ByteUnitMultiplier to src/util/strencodings.src/util/strencodings.h
Let me know what you think.

maflcko

feel free to ignore the style nits

src/util/strencodings.cpp

src/util/strencodings.h

ryanofsky

Code review ACK 1f51a25aa5037210ba39e14f048b3c9ec1d342ca. Main changes since last review are removing default multiplier, asserting multiplier is positive, adding enum

vasild

The code looks ok (1f51a25aa5037210ba39e14f048b3c9ec1d342ca). Some nits below, feel free to ignore.

I think the 3 commits should be squashed into one - subsequent ones just change stuff added by the first commit.

src/test/util_tests.cpp

src/util/strencodings.h

A convenience utility for human readable arguments/config e.g. -maxuploadtarget=500g

dougEfresh · 2021-11-17T10:53:10Z

@MarcoFalke
hopefully, last review 21b58f43
Main changes since last review are removing assigning a default multiplier, adding enum for ByteUnit(s)

vasild

ACK 21b58f4

Just squash and 2 minor nits since 1f51a25aa5, verified with

diff -u <(git diff 1f51a25aa5~3..1f51a25aa5) <(git show 21b58f430f) |vim -

Thanks!

ryanofsky

Code review ACK 21b58f4. Only changes since last review are dropping optional has_value call, fixing comment punctuation, squashing commits.

maflcko · 2021-11-24T09:47:53Z

src/test/util_tests.cpp

+    BOOST_CHECK_EQUAL(ParseByteUnits("1", noop).value(), 1);
+    BOOST_CHECK_EQUAL(ParseByteUnits("0", noop).value(), 0);
+
+    BOOST_CHECK_EQUAL(ParseByteUnits("1k", noop).value(), 1000ULL);


nit: Could also add a test that default_multiplier is ignored completely if passed in via the string? Maybe also add a fuzz test?

…x unit

rebroad · 2021-12-24T20:38:44Z

This fails to compile on some (unsupported, e.g. cygwin) systems:

  CXX      bitcoind-bitcoind.o
In file included from ./netaddress.h:19,
                 from ./chainparams.h:11,
                 from bitcoind.cpp:10:
./util/strencodings.h:41:10: warning: multi-character character constant [-Wmultichar]
   41 |     m = 1'000'000ULL,
      |          ^~~~~

sipa · 2021-12-24T20:53:51Z

@rebroad You need a C++17 compiler.

dougEfresh mentioned this pull request Oct 11, 2021

[RFC] util: Add suffix byte unit parser for ArgsManager #23224

Closed

DrahtBot added the Utils/log/libs label Oct 11, 2021

laanwj reviewed Oct 11, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

laanwj reviewed Oct 11, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

laanwj reviewed Oct 11, 2021

View reviewed changes

src/util/strencodings.h Outdated Show resolved Hide resolved

laanwj reviewed Oct 11, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch from 55868c8 to 7d96a50 Compare October 13, 2021 14:54

dougEfresh changed the title ~~util: ParseByteUnits - Parse a string with a suffix unit~~ util: ParseByteUnits - Parse a string with suffix unit Oct 13, 2021

DrahtBot mentioned this pull request Oct 14, 2021

doc: Remove TODO 'exclude peers with download permission' #23128

Merged

practicalswift reviewed Oct 15, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch 2 times, most recently from 933f7ff to 0d786a1 Compare October 15, 2021 17:04

promag reviewed Oct 15, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch from 0d786a1 to 8b16cb5 Compare October 16, 2021 11:02

dougEfresh force-pushed the util-parsebyteunit branch from 8b16cb5 to c5d4a59 Compare November 1, 2021 08:02

dougEfresh commented Nov 1, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Nov 2, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/test/util_tests.cpp Outdated Show resolved Hide resolved

src/test/util_tests.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch from c5d4a59 to f1f8a77 Compare November 2, 2021 22:05

dougEfresh requested a review from laanwj November 2, 2021 22:18

ryanofsky approved these changes Nov 8, 2021

View reviewed changes

vasild reviewed Nov 9, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/test/util_tests.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 9, 2021

net: respect -onlynet= when making outbound connections #22834

Merged

dougEfresh force-pushed the util-parsebyteunit branch 2 times, most recently from 6888756 to f3f8806 Compare November 9, 2021 12:17

vasild approved these changes Nov 9, 2021

View reviewed changes

src/util/strencodings.h Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch from f3f8806 to 6ebb476 Compare November 9, 2021 14:32

vasild approved these changes Nov 9, 2021

View reviewed changes

dougEfresh force-pushed the util-parsebyteunit branch from 6ebb476 to 4451a00 Compare November 9, 2021 16:46

vasild approved these changes Nov 10, 2021

View reviewed changes

promag reviewed Nov 10, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

maflcko reviewed Nov 10, 2021

View reviewed changes

dougEfresh force-pushed the util-parsebyteunit branch from 4451a00 to e4e2441 Compare November 10, 2021 16:10

maflcko reviewed Nov 10, 2021

View reviewed changes

src/util/strencodings.cpp Outdated Show resolved Hide resolved

src/util/strencodings.cpp Outdated Show resolved Hide resolved

dougEfresh commented Nov 12, 2021

View reviewed changes

src/util/strencodings.h Outdated Show resolved Hide resolved

promag reviewed Nov 12, 2021

View reviewed changes

src/util/strencodings.h Outdated Show resolved Hide resolved

dougEfresh force-pushed the util-parsebyteunit branch 2 times, most recently from 7f101cc to 02df252 Compare November 13, 2021 15:16

ryanofsky approved these changes Nov 15, 2021

View reviewed changes

vasild reviewed Nov 17, 2021

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

src/util/strencodings.h Outdated Show resolved Hide resolved

util: ParseByteUnits - Parse a string with suffix unit [k|K|m|M|g|G|t|T]

21b58f4

A convenience utility for human readable arguments/config e.g. -maxuploadtarget=500g

dougEfresh force-pushed the util-parsebyteunit branch from 1f51a25 to 21b58f4 Compare November 17, 2021 10:49

vasild approved these changes Nov 17, 2021

View reviewed changes

ryanofsky approved these changes Nov 24, 2021

View reviewed changes

maflcko approved these changes Nov 24, 2021

View reviewed changes

maflcko merged commit 73ac195 into bitcoin:master Nov 24, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 24, 2021

Merge bitcoin#23249: util: ParseByteUnits - Parse a string with suffi…

9a3c515

…x unit

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 24, 2021

Merge bitcoin#23249: util: ParseByteUnits - Parse a string with suffi…

9ac3ebe

…x unit

delta1 added a commit to delta1/elements that referenced this pull request May 30, 2023

Merge 73ac195 into merged_master (Bitcoin PR bitcoin/bitcoin#23249)

3f0bf16

bitcoin locked and limited conversation to collaborators Sep 21, 2023

Conversation

dougEfresh commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougEfresh commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougEfresh commented Nov 2, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

dougEfresh commented Nov 9, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougEfresh Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dougEfresh commented Oct 11, 2021 •

edited

Loading

dougEfresh commented Oct 13, 2021 •

edited

Loading

DrahtBot commented Oct 14, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

dougEfresh Nov 10, 2021 •

edited

Loading