[PATCH 1/3] xenstored - postpone fork after initialization

[PATCH 1/3] xenstored - postpone fork after initialization

[Bastian Blank]

# HG changeset patch
# User Bastian Blank <waldi@debian.org>
# Date 1206528849 -3600
# Node ID 5e0412c5f5798b5c0acdd4057c91b3820d6c4afd
# Parent  f5e6cccfdda5537876d6fc2b87ea1124d6043fc8
Postpone fork after initialization.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff -r f5e6cccfdda5 -r 5e0412c5f579 tools/xenstore/xenstored_core.c
--- a/tools/xenstore/xenstored_core.c	Tue Mar 25 18:02:00 2008 +0000
+++ b/tools/xenstore/xenstored_core.c	Wed Mar 26 11:54:09 2008 +0100
@@ -1839,13 +1839,6 @@ int main(int argc, char *argv[])
 		}
 	}
 
-	if (dofork) {
-		openlog("xenstored", 0, LOG_DAEMON);
-		daemonize();
-	}
-	if (pidfile)
-		write_pidfile(pidfile);
-
 	/* Talloc leak reports go to stderr, which is closed if we fork. */
 	if (!dofork)
 		talloc_enable_leak_report_full();
@@ -1899,22 +1892,30 @@ int main(int argc, char *argv[])
 	/* Restore existing connections. */
 	restore_existing_connections();
 
-	if (outputpid) {
-		printf("%ld\n", (long)getpid());
-		fflush(stdout);
-	}
-
 	/* redirect to /dev/null now we're ready to accept connections */
 	if (dofork) {
 		int devnull = open("/dev/null", O_RDWR);
 		if (devnull == -1)
 			barf_perror("Could not open /dev/null\n");
+
+		openlog("xenstored", 0, LOG_DAEMON);
+
+		daemonize();
+
+		if (outputpid) {
+			printf("%ld\n", (long)getpid());
+			fflush(stdout);
+		}
+
 		dup2(devnull, STDIN_FILENO);
 		dup2(devnull, STDOUT_FILENO);
 		dup2(devnull, STDERR_FILENO);
 		close(devnull);
 		xprintf = trace;
 	}
+
+	if (pidfile)
+		write_pidfile(pidfile);
 
 	signal(SIGHUP, trigger_reopen_log);
 
-- 
History tends to exaggerate.
		-- Col. Green, "The Savage Curtain", stardate 5906.4

Re: [PATCH 1/3] xenstored - postpone fork after initialization

[Keir Fraser]

On 26/3/08 11:00, "Bastian Blank" <bastian@waldi.eu.org> wrote:

>  HG changeset patch
> # User Bastian Blank <waldi@debian.org>
> # Date 1206528849 -3600
> # Node ID 5e0412c5f5798b5c0acdd4057c91b3820d6c4afd
> # Parent  f5e6cccfdda5537876d6fc2b87ea1124d6043fc8
> Postpone fork after initialization.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>

I applied all three patches as c/s 17296 and then reverted them in c/s 17304
because they don't work. I think delaying xenstored daemonisation breaks its
startup. Certainly xend itself came up, but xenstore-ls for example did not
work, so clearly xenstored had not initialised properly.

In any case I think it's bad practice to daemonise after initialisation --
if you're going to daemonise it makes sense to do it as early as possible.
Perhaps a better way to go would be to wait for the stdout pipe to be
half-closed? Or to pass switch -P to xenstored and wait for the daemon pid
to be written to the stdout pipe?

 -- Keir

Re: [PATCH 1/3] xenstored - postpone fork after initialization

[Bastian Blank]

On Thu, Mar 27, 2008 at 09:15:55AM +0000, Keir Fraser wrote:
> In any case I think it's bad practice to daemonise after initialisation --

The daemons I just checked disagree with you. They want to provide
errors if the initialization fails, which is only possible before the
parent exits.

> Perhaps a better way to go would be to wait for the stdout pipe to be
> half-closed? Or to pass switch -P to xenstored and wait for the daemon pid
> to be written to the stdout pipe?

Well, find the real problem. In my tests it properly responds to the
clients.

Bastian

-- 
Killing is stupid; useless!
		-- McCoy, "A Private Little War", stardate 4211.8

Re: [PATCH 1/3] xenstored - postpone fork after initialization

[Keir Fraser]

By the way, were the xend startup changes required at all? getstatusoutput()
waits for the child process to exit, doesn't it?

 -- Keir

On 27/3/08 11:27, "Bastian Blank" <bastian@waldi.eu.org> wrote:

> On Thu, Mar 27, 2008 at 09:15:55AM +0000, Keir Fraser wrote:
>> In any case I think it's bad practice to daemonise after initialisation --
> 
> The daemons I just checked disagree with you. They want to provide
> errors if the initialization fails, which is only possible before the
> parent exits.
> 
>> Perhaps a better way to go would be to wait for the stdout pipe to be
>> half-closed? Or to pass switch -P to xenstored and wait for the daemon pid
>> to be written to the stdout pipe?
> 
> Well, find the real problem. In my tests it properly responds to the
> clients.
> 
> Bastian

Re: [PATCH 1/3] xenstored - postpone fork after initialization

[John Levon]

On Thu, Mar 27, 2008 at 09:15:55AM +0000, Keir Fraser wrote:

> In any case I think it's bad practice to daemonise after initialisation --
> if you're going to daemonise it makes sense to do it as early as possible.

FWIW it's not only good practice on Solaris but needed for 100% correct
SMF behaviour: when the startup method returns the service is expected
to be 100% ready to go

regards
john