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

1. 
   dougEfresh commented at 7:18 PM on October 7, 2021: contributor

   Allow arguments/config to have a suffix byte unit: [k|K] [m|M] [g|G] [t|T] This makes byte valued argument concise and easy to read.

   -maxuploadtarget=500000 is -maxuploadtarget=500g

   Question:

   • If we fail to parse a string should we indicate an error or use default value?
     This PR favours error indication by returning -1

   • I default to use MiB if no unit is specified, since most of the arguments are in MiB

2. util: Add suffix byte unit parser for ArgsManager

   Allow arguments/config to have a suffix byte unit: [k|K] [m|M] [g|G] [t|T]
   This makes byte valued argument concise and easy to read.
   
   -maxuploadtarget=500000 is -maxuploadtarget=500g

   c9ada5f5a4
3. in src/util/system.cpp:645 in c9ada5f5a4

    638 | @@ -601,6 +639,12 @@ bool ArgsManager::GetBoolArg(const std::string& strArg, bool fDefault) const
    639 |      return value.isNull() ? fDefault : value.isBool() ? value.get_bool() : InterpretBool(value.get_str());
    640 |  }
    641 |  
    642 | +int64_t ArgsManager::GetByteArg(const std::string& strArg, int64_t nDefault) const
    643 | +{
    644 | +    const util::SettingsValue value = GetSetting(strArg);
    645 | +    return value.isNull() ? nDefault : value.isNum() ? value.get_int64() : ParseByteUnits(value.get_str());
   

   ---

   ---

   

   dougEfresh commented at 7:24 PM on October 7, 2021:

   I took this from GetIntArg, but I feel like the value.isNum() is not necessary and could cause confusion

4. DrahtBot added the label Utils/log/libs on Oct 7, 2021
5. fanquake requested review from ryanofsky on Oct 8, 2021
6. 
   fanquake commented at 1:45 AM on October 8, 2021: member

   Concept ~0. Based solely on the fact that I think this has to be very well justified to make our argument parsing / command line options code even more complicated.

7. 
   dougEfresh commented at 11:36 AM on October 8, 2021: contributor

   @fanquake I agree. As i was looking at ArgsManager it was much more sophisticated than i imagine.

   I suppose I just want maxuploadtarget to be in GiB. i will close this

8. dougEfresh closed this on Oct 8, 2021

   ---

9. 
   ryanofsky commented at 9:48 PM on October 8, 2021: member

   I think the functionality and the ParseByteUnits function and tests all seem useful. Only the integration with ArgsManager is messy and probably premature (it would be better with #22766 / #16545 and stricter validation).

   But it would be straightforward to add the functionality here without changing ArgsManager, by moving the parse function to strencodings, and accessing it as ParseByteSize(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET)). Would also suggest changing the parse function to return optional<size_t> instead int64_t and -1.

10. 
    dougEfresh commented at 9:33 AM on October 9, 2021: contributor

    But it would be straightforward to add the functionality here without changing ArgsManager, by moving the parse function to strencodings, and accessing it as ParseByteSize(args.GetArg("-maxuploadtarget", DEFAULT_MAX_UPLOAD_TARGET)).

    Excellent idea, I'll open a different PR for discussion/review.

11. 
    dougEfresh commented at 10:27 AM on October 11, 2021: contributor

    @ryanofsky created a util method in strencodings: https://github.com/bitcoin/bitcoin/pull/23249

12. DrahtBot locked this on Oct 30, 2022

Review Requested
ryanofsky