Fix bitcoin-cli exit status code #3736

cozz commented at 1:40 PM on February 24, 2014: contributor

bitcoin-cli returns 1 on success and 0 on error. By convention it should be the other way round.

It is not possible to distinguish between different error codes, all errors return 0. But for an AppInitRPC error, we would return 1...

in src/bitcoin-cli.cpp:None in 99a169afd9 outdated

  69 | -        if(!CommandLineRPC(argc, argv))
  70 | -            return 1;
  71 | +        ret = CommandLineRPC(argc, argv);
  72 |      }
  73 |      catch (std::exception& e) {
  74 |          PrintExceptionContinue(&e, "CommandLineRPC()");

laanwj commented at 1:55 PM on February 24, 2014:

Maybe we should also return 1 in the case of an exception (so don't default ret to 0)

cozz commented at 2:02 PM on February 24, 2014:

Yes, I was also thinking about that, but I was not sure. So I will update the pull then.

cozz commented at 2:18 PM on February 24, 2014: contributor

update:

return 1 in the case of an exception

laanwj commented at 2:23 PM on February 24, 2014: member

ACK on code changes (didn't test yet)

laanwj added this to the milestone 0.9.0 on Feb 24, 2014

laanwj commented at 11:15 AM on February 25, 2014: member

$ src/bitcoin-cli help; echo $?
...
0

$ src/bitcoin-cli; echo $?
... [help message] ...
1

$ src/bitcoin-cli sendtoaddress "1234" 1.0; echo $?
error: {"code":-5,"message":"Invalid Bitcoin address"}
5

src/bitcoin-cli -rpcport=6667 help; echo $?
error: couldn't connect to server
87

ACK

laanwj commented at 2:18 PM on February 25, 2014: member

Nothing new in this pull, but I wonder why this returns 87. IMO it should simply be 1 (=-RPC_MISC_ERROR): https://github.com/bitcoin/bitcoin/blob/master/src/rpcclient.cpp#L236

mcassano commented at 11:29 PM on February 25, 2014: contributor

cozz: Can you refactor the 1 and 0 into RET_SUCCESS and RET_FAIL variables?

laanwj commented at 6:24 AM on February 26, 2014: member

RET_SUCCESS / RET_FAIL is not any POSIX or C standard is it? We're also not using them in any other place at the moment.

If you want to use constants, using abs(constant_from_RPCErrorCode) from src/rpcprotocol.h would make more sense as that's where the error code that are returned by CommandLineRPC (and thus by the -cli now) come from.

cozz commented at 11:41 AM on February 26, 2014: contributor

update:

replace 1 with abs(RPC_MISC_ERROR)
replace 87 with abs(RPC_MISC_ERROR)

We might break a script by changing this error code, but should be very unlikely.

BitcoinPullTester commented at 11:50 AM on February 26, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/875acdf78693332a1500557ee728a4341dbe2d4c for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

in src/rpcclient.cpp:None in 216f5ab6a4 outdated

 232 | @@ -233,10 +233,11 @@ int CommandLineRPC(int argc, char *argv[])
 233 |      }
 234 |      catch (std::exception& e) {
 235 |          strPrint = string("error: ") + e.what();
 236 | -        nRet = 87;
 237 | +        nRet = abs(RPC_MISC_ERROR);
 238 |      }
 239 |      catch (...) {
 240 |          PrintException(NULL, "CommandLineRPC()");
 241 | +        nRet = abs(RPC_MISC_ERROR);

laanwj commented at 12:06 PM on February 26, 2014:

Execution will never get here. PrintException does a re-throw unlike PrintExceptionContinue. I'd never code it that way and would put the throw in the catch() statement itself for clarity, but this is part of our legacy.

cozz commented at 12:15 PM on February 26, 2014:

I see. Removing this then.

laanwj commented at 12:07 PM on February 26, 2014: member

ACK on the abs(RPC_MISC_ERROR) change.

Fix bitcoin-cli exit status code a719903804

laanwj referenced this in commit 3480bf7c65 on Feb 26, 2014

laanwj merged this on Feb 26, 2014

laanwj closed this on Feb 26, 2014

DrahtBot locked this on Sep 8, 2021