bitcoind: Daemonize using daemon(3) #8813

Merged
merged 1 commit into from Sep 30, 2016

Projects

None yet

6 participants

@laanwj
Member
laanwj commented Sep 26, 2016 edited

Simplified version of #8278. Assumes that every OS that (a) is supported by Bitcoin Core (b) supports daemonization has the daemon() function in its C library.

  • Removes the fallback path for operating systems that support daemonization but not daemon(). This prevents never-exercised code from ending up in the repository (see discussion here:
    #8278 (comment)).
  • Removes the windows-specific path. Windows doesn't support daemon(), so it doesn't support daemonization there, automatically. Give an explicit error if a user specifies -daemon on an OS where this is not supported.
  • Also made showing the help message depend on HAVE_DECL_DAEMON instead of !WIN32.

The original problem reported in #8278 was "When started with cron, the sendmail process hung around waiting for its stdin, bitcoind's stdout/err, to close..".

@laanwj laanwj referenced this pull request Sep 26, 2016
Closed

Forking daemon #8278

src/bitcoind.cpp
- if (pid < 0)
- {
- fprintf(stderr, "Error: fork() returned %d errno %d\n", pid, errno);
+ // daemon(3) basically does exactly what follows in the else block,
@sipa
sipa Sep 26, 2016 Member

That's not true anymore.

@laanwj
laanwj Sep 26, 2016 Member

Two more lines that can be deleted, thanks.

@laanwj laanwj changed the title from bitcoind: Daemonise using daemon(3) to bitcoind: Daemonize using daemon(3) Sep 26, 2016
@ChoHag @laanwj ChoHag bitcoind: Daemonize using daemon(3)
Simplified version of #8278. Assumes that every OS that (a) is supported
by Bitcoin Core (b) supports daemonization has the `daemon()` function
in its C library.

- Removes the fallback path for operating systems that support
  daemonization but not `daemon()`. This prevents never-exercised code from
  ending up in the repository (see discussion here:
  bitcoin#8278 (comment)).

- Removes the windows-specific path. Windows doesn't support `daemon()`,
  so it don't support daemonization there, automatically.

Original code by Matthew King, adapted by Wladimir van der Laan.
a92bf4a
@jonasschnelli

utACK a92bf4a (-comment nit mentioned by @sipa)

@MarcoFalke
Member

Possibly related: #4676 (haven't checked)

@sipa
Member
sipa commented Sep 28, 2016

Concept ACK.

@laanwj
Member
laanwj commented Sep 28, 2016

Possibly related: #4676 (haven't checked)

Yes, using daemonize() seems quite a step closer to standard UNIX daemon practices. Though we had to decide in #8278 to use the nochdir flag as, without further changes, it would have effects on option behavior.

@laanwj laanwj merged commit a92bf4a into bitcoin:master Sep 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Sep 30, 2016
@laanwj laanwj Merge #8813: bitcoind: Daemonize using daemon(3)
a92bf4a bitcoind: Daemonize using daemon(3) (Matthew King)
fb24d7e
@laanwj
Member
laanwj commented Sep 30, 2016

Note: if you end up here seeing the "Error: -daemon is not supported on this operating system" message while your OS does support it, this most likely means you need to re-run autogen.sh and configure.

@paveljanik
Contributor

This brings new warning on OS X:

bitcoind.cpp:137:17: warning: 'daemon' is deprecated: first deprecated in OS X 10.5 [-Wdeprecated-declarations]
            if (daemon(1, 0)) {
                ^
/usr/include/stdlib.h:267:5: note: 'daemon' has been explicitly marked deprecated here
int daemon(int, int) __asm("_" "daemon" "$1050") __attribute__((availability(macosx,introduced=10.0,deprecated=10.5)));
    ^
1 warning generated.
@laanwj
Member
laanwj commented Oct 22, 2016

You can't be friggin serious.
I'm going to revert this - it's only causing trouble.

@laanwj
Member
laanwj commented Oct 22, 2016

@paveljanik Just curious do they suggest anything to use instead of daemon?

@jonasschnelli
Member
jonasschnelli commented Oct 22, 2016 edited

I wouldn't take the deprecation warning to serious.
OSX want devs to use its own launchd/launchctrl.
Removing the actual daemon() systemcall would break tons of applications and if - we would have at least 1-2 years to switch to launchd on OSX.

@laanwj
Member
laanwj commented Oct 22, 2016 edited

Apparently they recommend doing something Apple-specific with plists and such https://developer.apple.com/library/content/technotes/tn2083/_index.html#//%20apple_ref/doc/uid/DTS10003794-CH1-SUBSECTION64
Bah. Okay, reverting this isn't any help either then. Never mind that. I got upset.

@paveljanik
Contributor

I agree with @jonasschnelli. No need to revert this. This only shows how Apple deprecates stuff.
10.5. Deprecated since October 26, 2007, still works now ;-)

What is the current status of daemon in the current macOS?

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@ChoHag @luke-jr ChoHag + luke-jr bitcoind: Daemonize using daemon(3)
Simplified version of #8278. Assumes that every OS that (a) is supported
by Bitcoin Core (b) supports daemonization has the `daemon()` function
in its C library.

- Removes the fallback path for operating systems that support
  daemonization but not `daemon()`. This prevents never-exercised code from
  ending up in the repository (see discussion here:
  bitcoin#8278 (comment)).

- Removes the windows-specific path. Windows doesn't support `daemon()`,
  so it don't support daemonization there, automatically.

Original code by Matthew King, adapted by Wladimir van der Laan.

Github-Pull: #8813
Rebased-From: a92bf4a
377ab84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment