Fix exit codes

UdjinM6 commented at 7:20 pm on November 2, 2016: contributor

--help, --version etc should exit with 0 i.e. no error (“not enough args” case should still trigger an error)
error reading config file should exit with 1

in src/bitcoin-cli.cpp: in e6d819268d outdated

84@@ -85,7 +85,7 @@ static bool AppInitRPC(int argc, char* argv[])
85         }
86 
87         fprintf(stdout, "%s", strUsage.c_str());
88-        return false;
89+        if(argc < 2) return false; else exit(0);

laanwj commented at 7:31 pm on November 2, 2016:

Please don’t combine return and exit here.

UdjinM6 commented at 7:55 pm on November 2, 2016:

I don’t like that kind of code mix either but returning true here instead of exiting early fails with too few parameters (need at least command) at CommandLineRPC later.

0> ./src/bitcoin-cli --version; echo $?
1Bitcoin Core RPC client version v0.13.99.0-e6d8192
2error: too few parameters (need at least command)
31

laanwj commented at 7:59 pm on November 2, 2016:

I’m sure there’s other solutions though. You just need more return values and handle them at the call site. What about returning a pair<bool,int>?

<false, 1> to exit error
<false, 0> to exit success
<true,X> to continue

MarcoFalke commented at 9:48 pm on November 2, 2016:

Also, don’t combine 4 separate lines into one with no braces and nothing. Not only does it violate our coding guideline aesthetically; it also introduced really nasty bugs in the past.

UdjinM6 commented at 10:27 pm on November 2, 2016:

@laanwj I’m not sure if refactoring all nearby code would worth it. Wouldn’t it be better to just exit(0); here then i.e. to show help by default when no command was provided without any error at all?

laanwj commented at 8:51 am on November 3, 2016:

Well that’s a bit of “broken window fallacy” right there. Yes, the current code is a mess, but you’re changing it, please leave it in a better state than it is :) I certainly think the code is “worth refactoring” but that may be more work than you bargained for. In any case I don’t like your code change as it looks right now. Exit()-ing in a sub function should be left for urgent shutdown cases. An alternative to my (golang-ish) proposal to use return tuples may be using exceptions. E.g. make the AppInitRPC function return “true” on success (continue), “false” on success (printed help, quit now with 0), or raise an exception when something is wrong (catch it at call site, quit now with 1).

UdjinM6 commented at 11:48 am on November 3, 2016:

Ok, that makes sense. How about simply returning int instead of bool from AppInit*? And then

0        int ret = AppInitRPC(argc, argv);
1        if (ret != -1)
2            return ret;

This option seems a bit cleaner to me and should do the trick, see 0f9d7b6.

laanwj commented at 6:47 am on November 4, 2016:

I don’t think the code is very readable this way, there is nothing else that uses -1 as a success value.

You could improve it by defining a constant for success value (that happens to be -1, or whatever else), and document that the function returns either a process return code or this magic return success code.

MarcoFalke added the label Refactoring on Nov 3, 2016

laanwj commented at 2:21 pm on November 4, 2016: member

Looks good to me now, please squash your commits

Fix exit codes:

- `--help`, `--version` etc should exit with `0` i.e. no error ("not enough args" case should still trigger an error)
- error reading config file should exit with `1`

Slightly refactor AppInitRPC/AppInitRawTx to return standard exit codes (EXIT_FAILURE/EXIT_SUCCESS) or CONTINUE_EXECUTION (-1)

bd0de1386e

UdjinM6 commented at 3:48 pm on November 4, 2016: contributor

Squashed into bd0de13

in src/qt/bitcoin.cpp: in bd0de1386e outdated

572@@ -573,7 +573,7 @@ int main(int argc, char *argv[])
573     {
574         HelpMessageDialog help(NULL, mapArgs.count("-version"));
575         help.showOrPrint();
576-        return 1;
577+        return 0;

laanwj commented at 12:01 pm on November 7, 2016:

EXIT_SUCCESS?

UdjinM6 commented at 12:34 pm on November 7, 2016:

Technically, yes, but there are few other returns/exits in main() in src/qt/bitcoin.cpp (not to mention other places) and originally I tried to be consistent with the surrounding code, just fixing some parts to return correct value. Shall we replace all such magic numbers (here?) with constants then?

laanwj commented at 2:02 pm on November 7, 2016:

Dunno, it would make some sense to be consistent, but I don’t care deeply. But if you feel like changing it, this PR would be the chance to do it.

UdjinM6 commented at 6:36 pm on November 7, 2016:

done, see 4441018

Every main()/exit() should return/use one of EXIT_ codes instead of magic numbers 4441018d08

in src/qt/bitcoin.cpp: in bd0de1386e outdated

593@@ -594,7 +594,7 @@ int main(int argc, char *argv[])
594     } catch (const std::exception& e) {
595         QMessageBox::critical(0, QObject::tr(PACKAGE_NAME),
596                               QObject::tr("Error: Cannot parse configuration file: %1. Only use key=value syntax.").arg(e.what()));
597-        return false;
598+        return 1;

laanwj commented at 12:01 pm on November 7, 2016:

EXIT_FAILURE?

laanwj merged this on Nov 8, 2016

laanwj closed this on Nov 8, 2016

laanwj referenced this in commit f53023dbb8 on Nov 8, 2016

laanwj referenced this in commit 537e0cb252 on Nov 10, 2016

luke-jr referenced this in commit f85ee01303 on Dec 2, 2016

luke-jr referenced this in commit f27596a7ec on Dec 2, 2016

luke-jr referenced this in commit 08d1c90113 on Dec 2, 2016

laanwj referenced this in commit 423659c951 on Dec 14, 2016

dexX7 referenced this in commit 2fdbc202ae on Jan 9, 2017

dexX7 referenced this in commit b1c266f414 on Jan 9, 2017

dexX7 referenced this in commit afb0c58e0b on Jan 23, 2017

sickpig referenced this in commit 3cfdf2d3e7 on Mar 31, 2017

sickpig referenced this in commit 50b8c06272 on Mar 31, 2017

dexX7 referenced this in commit a400d26561 on Jun 8, 2017

dexX7 referenced this in commit 2476c64723 on Jun 8, 2017

dexX7 referenced this in commit 3edbe7cc79 on Jun 8, 2017

zkbot referenced this in commit 3b0a5bcd24 on Apr 13, 2018

zkbot referenced this in commit 65a8f9f201 on Apr 13, 2018

lateminer referenced this in commit 10ea8235a8 on Oct 16, 2018

lateminer referenced this in commit 4039e1c019 on Oct 16, 2018

DrahtBot locked this on Sep 8, 2021

Fix exit codes #9067