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.
1749 | @@ -1749,7 +1750,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1750 | connOptions.nReceiveFloodSize = 1000 * args.GetIntArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); 1751 | connOptions.m_added_nodes = args.GetArgs("-addnode"); 1752 | 1753 | - connOptions.nMaxOutboundLimit = 1024 * 1024 * args.GetIntArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET); 1754 | + connOptions.nMaxOutboundLimit = ParseByteUnits(args.GetArg("-maxuploadtarget", "")).value_or(DEFAULT_MAX_UPLOAD_TARGET);
When we change this, it would be nice to do actual error handling instead of silently using the default if the value is unparseable.
The current/master branch implementation is silent, I agree we should do some type of error handling, should we:
InitError()If we throw an InitError, some people won't notice it, as this part of the code comes after level DB initialize. The error is shown like ~45-60 seconds after start up. (I know I didn't notice it myself)
ryanofsky has other PR(s) that seems to address this by refactoring ArgsManager #22766 / #16545 . Perhaps wait for that.
ryanofsky has other PR(s) that seems to address this by refactoring ArgsManager #22766 / #16545 . Perhaps wait for that.
I'd definitely suggest fatal, explicit InitError for any bad or ambiguous values provided. ArgsManager class and PRs are a mess, I'd stay far away and keep this separate!
EDIT: More concretely, I'd suggest changing DEFAULT_MAX_UPLOAD_TARGET to a string and writing something like:
const std::string DEFAULT_MAX_UPLOAD_TARGET = "0M";
...
auto max_upload_str = args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET);
auto max_upload = ParseByteUnits(max_upload_str);
if (!max_upload) {
return InitError(strprintf("Bad -maxuploadtarget %s", max_upload_str));
}
connOptions.nMaxOutboundLimit = *max_upload;
537 | + size_t nBytes{0}; 538 | + 539 | + v.pop_back(); 540 | + switch (ToLower(unit)) { 541 | + case 'k': 542 | + return ParseUInt64(v, &nBytes) ? std::optional<size_t>{nBytes << 10} : std::nullopt;
Could factor out the ParseUInt64(v, &nBytes) part here, it's the same for all cases.
310 | + * 311 | + * Examples: 4m,6M,9g,78T 312 | + * 313 | + * @param[in] str the string to convert into bytes 314 | + * @param[in] default_multiplier if no unit is found in str, use this multiplier. default is MiB 315 | + * @returns optional size_t bytes from str
Please document under which conditions it returns a value or nullopt (e.g. parse errors).
546 | + break; 547 | + case 'g': 548 | + return ParseUInt64(v, &nBytes) ? std::optional<size_t>{nBytes << 30} : std::nullopt; 549 | + break; 550 | + case 't': 551 | + return ParseUInt64(v, &nBytes) ? std::optional<size_t>{nBytes << 40} : std::nullopt;
Are people going to fight over factor 1000 versus 1024 or has this been settled?
I'd support both, lowercase for 1000-based, uppercase for 1024-based? That hopefully avoids most controversy.
Also, this looks like it could easily overflow.
Updated with
size_t in favor of uint64_t (windows/mac didn't play nice with size_t)InitError on parsing failureWhat I need help with is overflows. I use __builtin_mul_overflow but there must be a better/easier solution.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
573 | + // use default_multipiler 574 | + break; 575 | + } 576 | + 577 | + int64_t total{0}; 578 | +#if defined(HAVE_BUILTIN_MUL_OVERFLOW)
Could use MultiplicationOverflow instead if it is moved out from src/test/fuzz/util.h and proper unit tests are added fot it?
Since I'm checking an unsigned int, I used
nBytes > std::numeric_limits<uint64_t>::max() / multiplier
If there is interest, I can create another PR with MultiplicationOverflow out of src/test/fuzz/util.h
537 | + auto unit = v.back(); 538 | + uint64_t nBytes{0}; 539 | + uint64_t multiplier{default_multiplier}; 540 | + 541 | + v.pop_back(); 542 | + if (!(ParseUInt64(v, &nBytes) || ParseUInt64(str, &nBytes))) {
I think parsing fails for number > 9 without unit?
Added this test case:
BOOST_CHECK_EQUAL(ParseByteUnits("10").value_or(0), 10ULL << 20UL);
568 | + break; 569 | + case 'T': 570 | + multiplier = 1ULL << 40; 571 | + break; 572 | + default: 573 | + // use default_multipiler
Should be digit?
Updated to parse the original str in the default case
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.
1749 | @@ -1749,7 +1750,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1750 | connOptions.nReceiveFloodSize = 1000 * args.GetIntArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); 1751 | connOptions.m_added_nodes = args.GetArgs("-addnode"); 1752 | 1753 | - connOptions.nMaxOutboundLimit = 1024 * 1024 * args.GetIntArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET); 1754 | + auto opt_max_upload = ParseByteUnits(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET)); 1755 | + if (!opt_max_upload) {
I want to point out to reviewers that this changes start up behavior. Previously, -maxuploadtarget would silently fail and use DEFAULT_MAX_UPLOAD_TARGET.
This change will throw an error upon parse failure, but only after 30-50 seconds after start-up
re: #23249 (review)
This change will throw an error upon parse failure, but only after 30-50 seconds after start-up
I'm not seeing this take 30-50 seconds, just a second or two. Still it would be a good to provide faster checking, and just moving the parsing call up a little higher in the AppInitMain function seems to give good results. I tried:
diff --git a/src/init.cpp b/src/init.cpp
index 517ad2342ff..513cc24faeb 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1114,6 +1114,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
{
const ArgsManager& args = *Assert(node.args);
const CChainParams& chainparams = Params();
+
+ auto opt_max_upload = ParseByteUnits(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET));
+ if (!opt_max_upload) {
+ return InitError(strprintf(_("Unable to parse -maxuploadtarget: '%s' (possible integer overflow?)"), args.GetArg("-maxuploadtarget","")));
+ }
+
// ********************************************************* Step 4a: application initialization
if (!CreatePidFile(args)) {
// Detailed error printed inside CreatePidFile().
@@ -1749,11 +1755,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
connOptions.nSendBufferMaxSize = 1000 * args.GetIntArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER);
connOptions.nReceiveFloodSize = 1000 * args.GetIntArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER);
connOptions.m_added_nodes = args.GetArgs("-addnode");
-
- auto opt_max_upload = ParseByteUnits(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET));
- if (!opt_max_upload) {
- return InitError(strprintf(_("Unable to parse -maxuploadtarget: '%s' (possible integer overflow?)"), args.GetArg("-maxuploadtarget","")));
- }
connOptions.nMaxOutboundLimit = *opt_max_upload;;
connOptions.m_peer_connect_timeout = peer_connect_timeout;
Moved
583 | + 584 | + if (multiplier == 0) { 585 | + return opt_uint64{nBytes}; 586 | + } 587 | + // check overflow 588 | + return nBytes > std::numeric_limits<uint64_t>::max() / multiplier ? std::nullopt : opt_uint64{nBytes * multiplier};
In commit "util: ParseByteUnits - Parse a string with suffix unit [k|K] [m|M] [g|G] [t|T]" (c5d4a59cf91f54a2027851b32cf83cfcb04cd210)
There's repetition and unneeded variables in this function. Also nBytes holds the number before multiplication instead of the actual number of bytes, and the name doesn't follow current coding convention. Would suggest an overall cleanup here like:
diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
index c490cc7f3e1..0075e69c5e7 100644
--- a/src/util/strencodings.cpp
+++ b/src/util/strencodings.cpp
@@ -530,19 +530,8 @@ std::string HexStr(const Span<const uint8_t> s)
std::optional<uint64_t> ParseByteUnits(const std::string& str, const uint64_t default_multiplier)
{
- if (str.empty()) {
- return std::nullopt;
- }
-
- using opt_uint64 = std::optional<uint64_t>;
-
- uint64_t nBytes{0};
- uint64_t multiplier{default_multiplier};
- auto unit = str.back();
- auto v = str;
-
- v.pop_back();
-
+ auto multiplier = default_multiplier ? default_multiplier : 1;
+ char unit = str.empty() ? 0 : str.back();
switch (unit) {
case 'k':
multiplier = 1000;
@@ -569,21 +558,13 @@ std::optional<uint64_t> ParseByteUnits(const std::string& str, const uint64_t de
multiplier = 1ULL << 40;
break;
default:
- // unit should be digit, parse original str
- if (!ParseUInt64(str, &nBytes)) {
- return std::nullopt;
- }
- //check overflow and use default_multiplier
- return default_multiplier == 0 ? opt_uint64{nBytes} : nBytes > std::numeric_limits<uint64_t>::max() / default_multiplier ? std::nullopt : opt_uint64{nBytes * default_multiplier};
+ unit = 0;
+ break;
}
-
- if (!ParseUInt64(v, &nBytes)) {
+ uint64_t num;
+ if (!ParseUInt64(unit ? str.substr(0, str.size()-1) : str, &num) ||
+ num > std::numeric_limits<uint64_t>::max() / multiplier) {
return std::nullopt;
}
-
- if (multiplier == 0) {
- return opt_uint64{nBytes};
- }
- // check overflow
- return nBytes > std::numeric_limits<uint64_t>::max() / multiplier ? std::nullopt : opt_uint64{nBytes * multiplier};
+ return num * multiplier;
}
Great suggestion, code now looks cleaner.
2473 | + 2474 | + BOOST_CHECK_EQUAL(ParseByteUnits("4t").value_or(0), 4ULL * 1000 * 1000 * 1000 * 1000); 2475 | + BOOST_CHECK_EQUAL(ParseByteUnits("4T").value_or(0), 4ULL << 40); 2476 | + 2477 | + // override default multiplier 2478 | + BOOST_CHECK_EQUAL(ParseByteUnits("5", 1ULL << 10).value_or(0), 5ULL << 10);
In commit "util: ParseByteUnits - Parse a string with suffix unit [k|K] [m|M] [g|G] [t|T]" (c5d4a59cf91f54a2027851b32cf83cfcb04cd210)
Would be good to check behavior with 0 multiplier to make sure it doesn't accidentally change in the future.
Added 0 multiplier test
2486 | + 2487 | + // be positive 2488 | + BOOST_CHECK(!ParseByteUnits("-123m").has_value()); 2489 | + 2490 | + // zero padding 2491 | + BOOST_CHECK_EQUAL(ParseByteUnits("020M").value_or(1), 20ULL << 20);
In commit "util: ParseByteUnits - Parse a string with suffix unit [k|K] [m|M] [g|G] [t|T]" (c5d4a59cf91f54a2027851b32cf83cfcb04cd210)
Minor inconsistency here where this case is using .value_or(1) and cases above are using .value_or(0). I think it would best if all of these cases were consistent and better if they were using .value() instead of .value_or()
Agreed
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.
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
Code review ACK f1f8a772b0daf8cc0c40f0fc54e82618ad242ddd. Just suggested changes since last review (thanks!)
1754 | @@ -1748,8 +1755,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1755 | connOptions.nSendBufferMaxSize = 1000 * args.GetIntArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER); 1756 | connOptions.nReceiveFloodSize = 1000 * args.GetIntArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER); 1757 | connOptions.m_added_nodes = args.GetArgs("-addnode"); 1758 | - 1759 | - connOptions.nMaxOutboundLimit = 1024 * 1024 * args.GetIntArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET); 1760 | + connOptions.nMaxOutboundLimit = *opt_max_upload;;
nit: double ;; at the end
removed
2455 | @@ -2456,4 +2456,49 @@ BOOST_AUTO_TEST_CASE(remove_prefix) 2456 | BOOST_CHECK_EQUAL(RemovePrefix("", ""), ""); 2457 | } 2458 | 2459 | +BOOST_AUTO_TEST_CASE(util_ParseByteUnits)
Maybe add a test for unknown suffix, e.g. 123X.
Added invalid unit test
561 | + unit = 0; 562 | + break; 563 | + } 564 | + 565 | + uint64_t num; 566 | + if (!ParseUInt64(unit ? str.substr(0, str.size()-1) : str, &num) ||
nit: missing spaces around -.
When I added the space, it didn't look aesthetically pleasing to me, normally I do add a space, but this if statement looks better without I don't mind adding a space if you like or if someone else asks.
The thing is that lots of people have lots of different preferences. "Aesthetically pleasant" for somebody may not be for another. The project uses clang-format to avoid endless low-value discussions about styling.
@vasild I changed const uint64_t to uint64_t
435 | @@ -435,7 +436,7 @@ void SetupServerArgs(ArgsManager& argsman) 436 | argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); 437 | argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); 438 | argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); 439 | - argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'download' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION); 440 | + argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB, GiB or TiB per 24h). Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [m|M] MiB, [g|G] GiB, [t|T] TiB. Lowercase is 1000 base while uppercase is 1024 base (default).", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
I find this text confusing:
Optional suffix units [m|M] MiB, [g|G] GiB, [t|T] TiB. Lowercase is 1000 base while uppercase is 1024 base
Maybe:
Optional suffix units [m|M|g|G|t|T]. Lowercase is 1000 base while uppercase is 1024 base
@vasild Good suggestion, I've changed the doc to:
Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base
314 | + * @param[in] str the string to convert into bytes 315 | + * @param[in] default_multiplier if no unit is found in str, use this multiplier. default is MiB 316 | + * @returns optional uint64_t bytes from str or nullopt 317 | + * if ParseUInt64 is false, str is empty, trailing whitespace or overflow 318 | + */ 319 | +std::optional<uint64_t> ParseByteUnits(const std::string& str, const uint64_t default_multiplier = 1024UL*1024UL);
nit: drop unnecessary const from the integer argument
style: missing spaces around *:
std::optional<uint64_t> ParseByteUnits(const std::string& str, uint64_t default_multiplier = 1024UL * 1024UL);
I ran clang-format-diff.py
The const is still there. I think there was discussion somewhere about it, but I can't find it now...
563 | + } 564 | + 565 | + uint64_t num; 566 | + if (!ParseUInt64(unit ? str.substr(0, str.size()-1) : str, &num) || 567 | + num > std::numeric_limits<uint64_t>::max() / multiplier) { // check overflow 568 | + return std::nullopt;
style: this has 5 spaces for indentation, should be 4.
531 | +std::optional<uint64_t> ParseByteUnits(const std::string& str, const uint64_t default_multiplier) 532 | +{ 533 | + auto multiplier = default_multiplier ? default_multiplier : 1; 534 | + char unit = str.empty() ? 0 : str.back(); 535 | + switch (unit) { 536 | + case 'k':
style: case should be aligned below switch:
switch (unit) {
case 'k':
multiplier = 1000;
1114 | @@ -1114,6 +1115,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) 1115 | { 1116 | const ArgsManager& args = *Assert(node.args); 1117 | const CChainParams& chainparams = Params(); 1118 | + 1119 | + auto opt_max_upload = ParseByteUnits(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET)); 1120 | + if (!opt_max_upload) { 1121 | + return InitError(strprintf(_("Unable to parse -maxuploadtarget: '%s' (possible integer overflow?)"), args.GetArg("-maxuploadtarget","")));
style: missing space
return InitError(strprintf(_("Unable to parse -maxuploadtarget: '%s' (possible integer overflow?)"), args.GetArg("-maxuploadtarget", "")));
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.
ACK 6ebb47626ee30576548e8b3c01ad961761e28562
@MarcoFalke would you mind reviewing? Thanks
ACK 4451a0000dda5daba4b1624dce1c95f1914e7251
526 | @@ -526,3 +527,45 @@ std::string HexStr(const Span<const uint8_t> s) 527 | assert(it == rv.end()); 528 | return rv; 529 | } 530 | + 531 | +std::optional<uint64_t> ParseByteUnits(const std::string& str, uint64_t default_multiplier) 532 | +{ 533 | + auto multiplier = default_multiplier ? default_multiplier : 1;
nit, why not assert(default_multiplier > 0)?
Depends on how strict or how we want to use this function.
For me, a 0 default_multiplier means "don not apply a multiplier" if no unit found, parse str as is. This may not be intuitive.
I'm fine with restricting multiplier > 0.
@MarcoFalke what's your opinion ?
either way, I'll update the docs in strencoding.h to explaining what a default_multiplier of zero means.
Wouldn't "don not apply a multiplier" mean 1?
ok, i will add an assert
526 | @@ -526,3 +527,45 @@ std::string HexStr(const Span<const uint8_t> s) 527 | assert(it == rv.end()); 528 | return rv; 529 | } 530 | + 531 | +std::optional<uint64_t> ParseByteUnits(const std::string& str, uint64_t default_multiplier) 532 | +{ 533 | + auto multiplier = default_multiplier ? default_multiplier : 1; 534 | + char unit = str.empty() ? 0 : str.back();
nit,
if (str.empty()) return std::nullopt;
char unit = str.back();
Code review ACK 4451a0000dda5daba4b1624dce1c95f1914e7251.
561 | + unit = 0; 562 | + break; 563 | + } 564 | + 565 | + uint64_t num; 566 | + if (!ParseUInt64(unit ? str.substr(0, str.size() - 1) : str, &num) ||
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 :)
314 | + * @param[in] str the string to convert into bytes 315 | + * @param[in] default_multiplier if no unit is found in str, use this multiplier. default is MiB 316 | + * @returns optional uint64_t bytes from str or nullopt 317 | + * if ParseUInt64 is false, str is empty, trailing whitespace or overflow 318 | + */ 319 | +std::optional<uint64_t> ParseByteUnits(const std::string& str, uint64_t default_multiplier = 1024UL * 1024UL);
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.
548 | + break; 549 | + case 'M': 550 | + multiplier = 1ULL << 20; 551 | + break; 552 | + case 'g': 553 | + multiplier = 1000ULL * 1000 * 1000;
multiplier = 1000'000'000ULL;
Haven't tried this, but can the compiler parse this?
it does!
566 | + break; 567 | + } 568 | + 569 | + auto parsed_num = ToIntegral<uint64_t>(unit ? str.substr(0, str.size() - 1) : str); 570 | + // check overflow 571 | + return !parsed_num || parsed_num > std::numeric_limits<uint64_t>::max() / multiplier ? std::nullopt : std::optional{*parsed_num * multiplier};
if( !parsed_num || parsed_num > std::numeric_limits<uint64_t>::max() / multiplier){
return std::nullopt;
}
return *parsed_num * multiplier;
Maybe split up into two lines to avoid the std::optional{} constructor for the value?
I agree, it looks better split
feel free to ignore the style nits
41 | + M_IEC = 1ULL << 20, 42 | + G_SI = 1'000'000'000ULL, 43 | + G_IEC = 1ULL << 30, 44 | + T_SI = 1'000'000'000'000ULL, 45 | + T_IEC = 1ULL << 40, 46 | +};
I use the "technical" terms but I don't think they are intuitive, open to suggestions.
28 | @@ -29,6 +29,22 @@ enum SafeChars 29 | SAFE_CHARS_URI, //!< Chars allowed in URIs (RFC 3986) 30 | }; 31 | 32 | +/** 33 | + * Used by ParseByteUnits() 34 | + * SI are 1000 base while IEC is 1024 base 35 | + */ 36 | +enum ByteUnitMultiplier {
Use enum class, which can be used like ByteUnit::g or ByteUnit::K.
changed. more intuitive, clear.
Code review ACK 1f51a25aa5037210ba39e14f048b3c9ec1d342ca. Main changes since last review are removing default multiplier, asserting multiplier is positive, adding enum
2496 | + 2497 | + // fractions not allowed 2498 | + BOOST_CHECK(!ParseByteUnits("0.5T", noop)); 2499 | + 2500 | + // overflow 2501 | + BOOST_CHECK(!ParseByteUnits("18446744073709551615g", noop).has_value());
nit: remove .has_value() for consistency with other tests:
BOOST_CHECK(!ParseByteUnits("18446744073709551615g", noop));
fixed in 21b58f430fa05fdb7c5db79b545302417a5dbceb
321 | @@ -305,4 +322,17 @@ std::string ToUpper(const std::string& str); 322 | */ 323 | std::string Capitalize(std::string str); 324 | 325 | +/** 326 | + * Parse a string with suffix unit [k|K|m|M|g|G|t|T] .
nit: extra white space
* Parse a string with suffix unit [k|K|m|M|g|G|t|T].
fixed
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.
A convenience utility for human readable arguments/config e.g. -maxuploadtarget=500g
@MarcoFalke hopefully, last review 21b58f43 Main changes since last review are removing assigning a default multiplier, adding enum for ByteUnit(s)
ACK 21b58f430fa05fdb7c5db79b545302417a5dbceb
Just squash and 2 minor nits since 1f51a25aa5, verified with
diff -u <(git diff 1f51a25aa5~3..1f51a25aa5) <(git show 21b58f430f) |vim -
Thanks!
Code review ACK 21b58f430fa05fdb7c5db79b545302417a5dbceb. Only changes since last review are dropping optional has_value call, fixing comment punctuation, squashing commits.
2462 | + 2463 | + // no multiplier 2464 | + BOOST_CHECK_EQUAL(ParseByteUnits("1", noop).value(), 1); 2465 | + BOOST_CHECK_EQUAL(ParseByteUnits("0", noop).value(), 0); 2466 | + 2467 | + 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?
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,
| ^~~~~