util: Disallow network-qualified command line options

ryanofsky commented at 8:34 pm on November 14, 2019: member

Previously these were allowed but ignored.

This change implements one of the settings simplifications listed in #17508. Change includes release notes.

in src/test/util_tests.cpp:1020 in baf24fc2ee outdated

908@@ -887,7 +909,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
909     // Results file is formatted like:
910     //
911     //   <input> || <IsArgSet/IsArgNegated/GetArg output> | <GetArgs output> | <GetUnsuitable output>
912-    BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
913+    BOOST_CHECK_EQUAL(out_sha_hex, "8fd4877bb8bf337badca950ede6c917441901962f160e52514e06a60dea46cde");

ryanofsky commented at 8:41 pm on November 14, 2019:

In commit “util: Disallow network-qualified command line options” (baf24fc2ee7b57d46dc57f18f840c6961a963540)

Effect of this change is to remove a lot of lines that contain network-qualified command line options from test output:

0-net=test -main.server=a1 -main.server=a2 noserver=1 main.server=c1 main.server=c2 server=c3 server=c4 soft || c3 | c3 c4

Lines without network-qualified command line options are unaffected:

0 net=test -server=a1 -server=a2 main.server=c1 main.server=c2 noserver=1 server=c3 server=c4 soft || a2 | a1 a2 c3 c4

ryanofsky force-pushed on Nov 14, 2019

DrahtBot added the label Docs on Nov 14, 2019

DrahtBot added the label Tests on Nov 14, 2019

DrahtBot added the label Utils/log/libs on Nov 14, 2019

DrahtBot commented at 9:56 pm on November 14, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

in src/util/system.cpp:318 in 124841fd56 outdated

339-                m_settings.command_line_options[key].push_back(value);
340-            }
341-        } else {
342-            error = strprintf("Invalid parameter -%s", key);
343+
344+        if (!flags || !section.empty()) {

promag commented at 10:03 pm on November 14, 2019:

nit, could have a comment like “command line options unknown or with section prefixes are invalid”

promag commented at 10:12 pm on November 14, 2019:

nit, alternatively could have a detailed error, like:

0if (!flags) {
1    error = strprintf("Unknown parameter %s", key);
2    return false;
3} else if (!section.empty()) {
4    error = strprintf("Invalid parameter %s, section %s now allowed", argv[i], section);
5    return false;
6}

ryanofsky commented at 1:34 pm on November 15, 2019:

re: #17482 (review)

nit, could have a comment like “command line options unknown or with section prefixes are invalid”

Thanks added comment.

nit, alternatively could have a detailed error, like:

0if (!flags) {
1    error = strprintf("Unknown parameter %s", key);
2    return false;
3} else if (!section.empty()) {
4    error = strprintf("Invalid parameter %s, section %s now allowed", argv[i], section);
5    return false;
6}

That error message seems worse to me. In the first branch, it provides less information and could make the bad argument harder to identify, and in the second branch it references the “section” of a command line parameter which isn’t actually a thing that exists, or someone would expect to exist.

in src/test/util_tests.cpp:350 in 124841fd56 outdated

237+    TestArgsManager test;
238+    test.SetupArgs({{"-registered", ArgsManager::ALLOW_ANY}});
239+
240+    const char* argv[] = {"ignored", "-registered"};
241+    std::string error;
242+    BOOST_CHECK(test.ParseParameters(2, (char**)argv, error));

promag commented at 10:05 pm on November 14, 2019:

nit, success case seems unnecessary.

ryanofsky commented at 1:28 pm on November 15, 2019:

re: #17482 (review)

nit, success case seems unnecessary.

It helps ensure the test passes for right reason. That ParseParameters calls below aren’t returning false for some other reason. Generally it’s nice when test cases vary one input at a time, so you can be sure about what causes the observed output.

fanquake removed the label Docs on Nov 14, 2019

in src/util/system.cpp:324 in 124841fd56 outdated

335-            // line options with section prefixes are allowed but ignored. It
336-            // would be better if these options triggered the Invalid parameter
337-            // error below.
338-            if (section.empty()) {
339-                m_settings.command_line_options[key].push_back(value);
340-            }

promag commented at 10:07 pm on November 14, 2019:

Previously a warning could make sense.

promag commented at 10:12 pm on November 14, 2019: member

Concept ACK, seems fine to do this.

laanwj commented at 9:48 am on November 15, 2019: member

Being explicit on invalid arguments is always better. And I don’t see any reason to implement them, so Concept ACK.

ariard approved

ariard commented at 5:19 pm on November 19, 2019: member

Code review and tested ACK 124841f

Tested network-qualified command line args with/without this patchset and errors are now thrown as expected.

ryanofsky force-pushed on Nov 21, 2019

ryanofsky commented at 10:46 pm on November 21, 2019: member

Updated 124841fd56e0d7f291f23fb2e02d4d2080e59e6a -> 5e4ef31fc6695bd93c4226d7f50efbd27d729222 (pr/wdqual.2 -> pr/wdqual.3, compare) just adding suggested comment

DrahtBot added the label Needs rebase on Dec 19, 2019

util: Disallow network-qualified command line options

Previously these were allowed but ignored.

900d8f6f70

ryanofsky force-pushed on Dec 19, 2019

DrahtBot removed the label Needs rebase on Dec 19, 2019

ajtowns commented at 2:38 am on January 9, 2020: member

Concept ACK

laanwj commented at 2:53 pm on February 5, 2020: member

ACK 900d8f6f70859f528e84c5c38d0332f81d19df55

laanwj referenced this in commit 8a56f79d49 on Feb 5, 2020

laanwj merged this on Feb 5, 2020

laanwj closed this on Feb 5, 2020

sidhujag referenced this in commit ac84b589c1 on Feb 9, 2020

sidhujag referenced this in commit 9166566ec7 on Nov 10, 2020

Fabcien referenced this in commit 13a518d4c0 on Dec 22, 2020

DrahtBot locked this on Feb 15, 2022

util: Disallow network-qualified command line options #17482

Conflicts