2007-08-17 Havoc Pennington <hp@redhat.com>

	* tools/dbus-launch-x11.c (set_address_in_x11): fix from Michael
	Lorenz to use long not int with XChangeProperty format 32

	* dbus/dbus-sysdeps-util-unix.c
	(_dbus_write_pid_to_file_and_pipe): factor this out, and use the
	same code in _dbus_become_daemon (where the parent writes the pid
	file and to the pid pipe) and in bus_context_new (where the daemon
	writes its own pid file and to its own pid pipe)

	* bus/bus.c (bus_context_new): close the pid pipe after we print
	to it. Also, don't write the pid to the pipe twice when we fork,
	someone reported this bug a long time ago.

6 files changed, 160 insertions, 135 deletions

diff --git a/ChangeLog b/ChangeLog
index 37ab97f9..0b44bc1b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2007-08-17  Havoc Pennington  <hp@redhat.com>
+
+	* tools/dbus-launch-x11.c (set_address_in_x11): fix from Michael
+	Lorenz to use long not int with XChangeProperty format 32
+
+	* dbus/dbus-sysdeps-util-unix.c
+	(_dbus_write_pid_to_file_and_pipe): factor this out, and use the
+	same code in _dbus_become_daemon (where the parent writes the pid
+	file and to the pid pipe) and in bus_context_new (where the daemon
+	writes its own pid file and to its own pid pipe)
+
+	* bus/bus.c (bus_context_new): close the pid pipe after we print
+	to it. Also, don't write the pid to the pipe twice when we fork,
+	someone reported this bug a long time ago.
+
 2007-08-03  Havoc Pennington  <hp@redhat.com>
 
 	* configure.in: add major/minor/micro version number AC_SUBST
diff --git a/bus/bus.c b/bus/bus.c
index 21e8037e..c2ea1157 100644
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -665,7 +665,7 @@ bus_context_new (const DBusString *config_file,
 
       if (!_dbus_pipe_is_stdout_or_stderr (print_addr_pipe))
         _dbus_pipe_close (print_addr_pipe, NULL);
-
+      
       _dbus_string_free (&addr);
     }
   
@@ -695,78 +695,48 @@ bus_context_new (const DBusString *config_file,
         }
     }
 
-  /* Now become a daemon if appropriate */
-  if ((force_fork != FORK_NEVER && context->fork) || force_fork == FORK_ALWAYS)
-    {
-      DBusString u;
-
-      if (context->pidfile)
-        _dbus_string_init_const (&u, context->pidfile);
-      
-      if (!_dbus_become_daemon (context->pidfile ? &u : NULL, 
-				print_pid_pipe,
-				error))
-	{
-	  _DBUS_ASSERT_ERROR_IS_SET (error);
-	  goto failed;
-	}
-    }
-  else
-    {
-      /* Need to write PID file for ourselves, not for the child process */
-      if (context->pidfile != NULL)
-        {
-          DBusString u;
-
-          _dbus_string_init_const (&u, context->pidfile);
-          
-          if (!_dbus_write_pid_file (&u, _dbus_getpid (), error))
-	    {
-	      _DBUS_ASSERT_ERROR_IS_SET (error);
-	      goto failed;
-	    }
-        }
-    }
-
-  /* Write PID if requested */
-  if (print_pid_pipe != NULL && _dbus_pipe_is_valid (print_pid_pipe))
-    {
-      DBusString pid;
-      int bytes;
-
-      if (!_dbus_string_init (&pid))
-        {
-          BUS_SET_OOM (error);
-          goto failed;
-        }
-      
-      if (!_dbus_string_append_int (&pid, _dbus_getpid ()) ||
-          !_dbus_string_append (&pid, "\n"))
-        {
-          _dbus_string_free (&pid);
-          BUS_SET_OOM (error);
-          goto failed;
-        }
-
-      bytes = _dbus_string_get_length (&pid);
-      if (_dbus_pipe_write (print_pid_pipe, &pid, 0, bytes, error) != bytes)
-        {
-          /* pipe_write sets error on failure but not short write */
-          if (error != NULL && !dbus_error_is_set (error))
-            {
-              dbus_set_error (error, DBUS_ERROR_FAILED,
-                              "Printing message bus PID: did not write enough bytes\n");
-            }
-          _dbus_string_free (&pid);
-          goto failed;
-        }
-
-      if (!_dbus_pipe_is_stdout_or_stderr (print_pid_pipe))
-        _dbus_pipe_close (print_pid_pipe, NULL);
-      
-      _dbus_string_free (&pid);
-    }
-
+  /* Now become a daemon if appropriate and write out pid file in any case */
+  {
+    DBusString u;
+
+    if (context->pidfile)
+      _dbus_string_init_const (&u, context->pidfile);
+
+    if ((force_fork != FORK_NEVER && context->fork) || force_fork == FORK_ALWAYS)
+      {
+        _dbus_verbose ("Forking and becoming daemon\n");
+        
+        if (!_dbus_become_daemon (context->pidfile ? &u : NULL, 
+                                  print_pid_pipe,
+                                  error))
+          {
+            _DBUS_ASSERT_ERROR_IS_SET (error);
+            goto failed;
+          }
+      }
+    else
+      {
+        _dbus_verbose ("Fork not requested\n");
+        
+        /* Need to write PID file and to PID pipe for ourselves,
+         * not for the child process. This is a no-op if the pidfile
+         * is NULL and print_pid_pipe is NULL.
+         */
+        if (!_dbus_write_pid_to_file_and_pipe (context->pidfile ? &u : NULL,
+                                               print_pid_pipe,
+                                               _dbus_getpid (),
+                                               error))
+          {
+            _DBUS_ASSERT_ERROR_IS_SET (error);
+            goto failed;
+          }
+      }
+  }
+
+  if (print_pid_pipe && _dbus_pipe_is_valid (print_pid_pipe) &&
+      !_dbus_pipe_is_stdout_or_stderr (print_pid_pipe))
+    _dbus_pipe_close (print_pid_pipe, NULL);
+  
   if (!process_config_postinit (context, parser, error))
     {
       _DBUS_ASSERT_ERROR_IS_SET (error);
diff --git a/bus/main.c b/bus/main.c
index 3652935e..161de19c 100644
--- a/bus/main.c
+++ b/bus/main.c
@@ -452,6 +452,10 @@ main (int argc, char **argv)
       exit (1);
     }
 
+  /* bus_context_new() closes the print_addr_pipe and
+   * print_pid_pipe
+   */
+  
   setup_reload_pipe (bus_context_get_loop (context));
 
   _dbus_set_signal_handler (SIGHUP, signal_handler);
diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c
index 38b7b756..df967a38 100644
--- a/dbus/dbus-sysdeps-util-unix.c
+++ b/dbus/dbus-sysdeps-util-unix.c
@@ -62,6 +62,7 @@
  * @{
  */
 
+
 /**
  * Does the chdir, fork, setsid, etc. to become a daemon process.
  *
@@ -123,68 +124,27 @@ _dbus_become_daemon (const DBusString *pidfile,
       /* Get a predictable umask */
       _dbus_verbose ("setting umask\n");
       umask (022);
+
+      _dbus_verbose ("calling setsid()\n");
+      if (setsid () == -1)
+        _dbus_assert_not_reached ("setsid() failed");
+      
       break;
 
     default:
-      if (pidfile)
+      if (!_dbus_write_pid_to_file_and_pipe (pidfile, print_pid_pipe,
+                                             child_pid, error))
         {
-          _dbus_verbose ("parent writing pid file\n");
-          if (!_dbus_write_pid_file (pidfile,
-                                     child_pid,
-                                     error))
-            {
-              _dbus_verbose ("pid file write failed, killing child\n");
-              kill (child_pid, SIGTERM);
-              return FALSE;
-            }
+          _dbus_verbose ("pid file or pipe write failed: %s\n",
+                         error->message);
+          kill (child_pid, SIGTERM);
+          return FALSE;
         }
 
-      /* Write PID if requested */
-      if (print_pid_pipe != NULL && _dbus_pipe_is_valid (print_pid_pipe))
-	{
-	  DBusString pid;
-	  int bytes;
-	  
-	  if (!_dbus_string_init (&pid))
-	    {
-	      _DBUS_SET_OOM (error);
-              kill (child_pid, SIGTERM);
-	      return FALSE;
-	    }
-	  
-	  if (!_dbus_string_append_int (&pid, child_pid) ||
-	      !_dbus_string_append (&pid, "\n"))
-	    {
-	      _dbus_string_free (&pid);
-	      _DBUS_SET_OOM (error);
-              kill (child_pid, SIGTERM);
-	      return FALSE;
-	    }
-	  
-	  bytes = _dbus_string_get_length (&pid);
-	  if (_dbus_pipe_write (print_pid_pipe, &pid, 0, bytes, error) != bytes)
-	    {
-              /* _dbus_pipe_write sets error only on failure, not short write */
-              if (error != NULL && !dbus_error_is_set(error))
-                {
-                  dbus_set_error (error, DBUS_ERROR_FAILED,
-                                  "Printing message bus PID: did not write enough bytes\n");
-                }
-	      _dbus_string_free (&pid);
-              kill (child_pid, SIGTERM);
-	      return FALSE;
-	    }
-	  
-	  _dbus_string_free (&pid);
-	}
       _dbus_verbose ("parent exiting\n");
       _exit (0);
       break;
     }
-
-  _dbus_verbose ("calling setsid()\n");
-  if (setsid () == -1)
-    _dbus_assert_not_reached ("setsid() failed");
   
   return TRUE;
 }
@@ -198,7 +158,7 @@ _dbus_become_daemon (const DBusString *pidfile,
  * @param error return location for errors
  * @returns #FALSE on failure
  */
-dbus_bool_t
+static dbus_bool_t
 _dbus_write_pid_file (const DBusString *filename,
                       unsigned long     pid,
 		      DBusError        *error)
@@ -249,6 +209,84 @@ _dbus_write_pid_file (const DBusString *filename,
 }
 
 /**
+ * Writes the given pid_to_write to a pidfile (if non-NULL) and/or to a
+ * pipe (if non-NULL). Does nothing if pidfile and print_pid_pipe are both
+ * NULL.
+ *
+ * @param pidfile the file to write to or #NULL
+ * @param print_pid_pipe the pipe to write to or #NULL
+ * @param pid_to_write the pid to write out
+ * @param error error on failure
+ * @returns FALSE if error is set
+ */
+dbus_bool_t
+_dbus_write_pid_to_file_and_pipe (const DBusString *pidfile,
+                                  DBusPipe         *print_pid_pipe,
+                                  dbus_pid_t        pid_to_write,
+                                  DBusError        *error)
+{
+  if (pidfile)
+    {
+      _dbus_verbose ("writing pid file %s\n", _dbus_string_get_const_data (pidfile));
+      if (!_dbus_write_pid_file (pidfile,
+                                 pid_to_write,
+                                 error))
+        {
+          _dbus_verbose ("pid file write failed\n");
+          _DBUS_ASSERT_ERROR_IS_SET(error);
+          return FALSE;
+        }
+    }
+  else
+    {
+      _dbus_verbose ("No pid file requested\n");
+    }
+
+  if (print_pid_pipe != NULL && _dbus_pipe_is_valid (print_pid_pipe))
+    {
+      DBusString pid;
+      int bytes;
+
+      _dbus_verbose ("writing our pid to pipe %d\n", print_pid_pipe->fd_or_handle);
+      
+      if (!_dbus_string_init (&pid))
+        {
+          _DBUS_SET_OOM (error);
+          return FALSE;
+        }
+	  
+      if (!_dbus_string_append_int (&pid, pid_to_write) ||
+          !_dbus_string_append (&pid, "\n"))
+        {
+          _dbus_string_free (&pid);
+          _DBUS_SET_OOM (error);
+          return FALSE;
+        }
+	  
+      bytes = _dbus_string_get_length (&pid);
+      if (_dbus_pipe_write (print_pid_pipe, &pid, 0, bytes, error) != bytes)
+        {
+          /* _dbus_pipe_write sets error only on failure, not short write */
+          if (error != NULL && !dbus_error_is_set(error))
+            {
+              dbus_set_error (error, DBUS_ERROR_FAILED,
+                              "Printing message bus PID: did not write enough bytes\n");
+            }
+          _dbus_string_free (&pid);
+          return FALSE;
+        }
+	  
+      _dbus_string_free (&pid);
+    }
+  else
+    {
+      _dbus_verbose ("No pid pipe to write to\n");
+    }
+
+  return TRUE;
+}
+
+/**
  * Verify that after the fork we can successfully change to this user.
  *
  * @param user the username given in the daemon configuration
diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h
index ab1631b8..eadfb433 100644
--- a/dbus/dbus-sysdeps.h
+++ b/dbus/dbus-sysdeps.h
@@ -363,13 +363,16 @@ void        _dbus_print_backtrace  (void);
 dbus_bool_t _dbus_become_daemon   (const DBusString *pidfile,
                                    DBusPipe         *print_pid_pipe,
                                    DBusError        *error);
-dbus_bool_t _dbus_write_pid_file  (const DBusString *filename,
-                                   unsigned long     pid,
-                                   DBusError        *error);
+
 dbus_bool_t _dbus_verify_daemon_user    (const char *user);
 dbus_bool_t _dbus_change_to_daemon_user (const char *user,
                                          DBusError  *error);
 
+dbus_bool_t _dbus_write_pid_to_file_and_pipe (const DBusString *pidfile,
+                                              DBusPipe         *print_pid_pipe,
+                                              dbus_pid_t        pid_to_write,
+                                              DBusError        *error);
+
 /** A UNIX signal handler */
 typedef void (* DBusSignalHandler) (int sig);
 
diff --git a/tools/dbus-launch-x11.c b/tools/dbus-launch-x11.c
index dfb28445..927d8634 100644
--- a/tools/dbus-launch-x11.c
+++ b/tools/dbus-launch-x11.c
@@ -342,7 +342,7 @@ set_address_in_x11(char *address, pid_t pid)
 {
   char *current_address;
   Window wid;
-  int pid32;
+  unsigned long pid32; /* Xlib property functions want _long_ not 32-bit for format "32" */
   
   /* lock the X11 display to make sure we're doing this atomically */
   XGrabServer (xdisplay);
@@ -372,11 +372,6 @@ set_address_in_x11(char *address, pid_t pid)
   XChangeProperty (xdisplay, wid, address_atom, XA_STRING, 8, PropModeReplace,
                    (unsigned char *)address, strlen (address));
   pid32 = pid;
-  if (sizeof(pid32) != 4)
-    {
-      fprintf (stderr, "int is not 32 bits!\n");
-      exit (1);
-    }
   XChangeProperty (xdisplay, wid, pid_atom, XA_CARDINAL, 32, PropModeReplace,
                    (unsigned char *)&pid32, 1);