This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] nscd: Improved support for tracking startup failure in nscd service (#16639)


On Mon, Mar 03, 2014 at 11:57:40AM -0500, Carlos O'Donell wrote:
> OK to checkin. 
> 

Thanks, this is what I have finally pushed.  I did not update the NEWS
file on purpose - I'll do it when I push (after it is reviewed of
course) the second patch to update the service file.

Siddhesh

commit a53ab0d95927d1be1c352774552adc0d44c33211
Author: Siddhesh Poyarekar <siddhesh@redhat.com>
Date:   Mon Mar 3 22:51:39 2014 +0530

    nscd: Improved support for tracking startup failure in nscd service (BZ #16639)
    
    Currently, the nscd parent process parses commandline options and
    configuration, forks on startup and immediately exits with a success.
    If the child process encounters some error after this, it goes
    undetected and any services started up after it may have to repeatedly
    check to make sure that the nscd service did actually start up and is
    serving requests.
    
    To make this process more reliable, I have added a pipe between the
    parent and child process, through which the child process sends a
    notification to the parent informing it of its status.  The parent
    waits for this status and once it receives it, exits with the
    corresponding exit code.  So if the child service sends a success
    status (0), the parent exits with a success status.  Similarly for
    error conditions, the child sends the non-zero status code, which the
    parent passes on as the exit code.
    
    This, along with setting the nscd service type to forking in its
    systemd configuration file, allows systemd to be certain that the nscd
    service is ready and is accepting connections.

diff --git a/ChangeLog b/ChangeLog
index c366a01..2015c19 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2014-03-03  Siddhesh Poyarekar  <siddhesh@redhat.com>
+
+	[BZ #16639]
+	* nscd/connections.c (nscd_init): Call do_exit.
+	(start_threads): Call do_exit and notify_parent.
+	(begin_drop_privileges): Call do_exit.
+	(finish_drop_privileges): Likewise.
+	* nscd/selinux.c (preserve_capabilities): Likewise.
+	(install_real_capabilities): Likewise.
+	(nscd_selinux_enabled): Likewise.
+	(avc_create_thread): Likewise.
+	(avc_alloc_lock): Likewise.
+	(nscd_avc_init): Likewise.
+	* nscd/nscd.c (parent_fd): New static variable.
+	(main): Create a pipe between parent and child processes.
+	Skip closing parent_fd.
+	(monitor_child): New function.
+	(do_exit): Likewise.
+	(notify_parent): Likewise.
+	* nscd/nscd.h (notify_parent): Likewise.
+	(do_exit): Likewise.
+
 2014-03-03  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
 
 	* sysdeps/powerpc/fpu/libm-test-ulps: Update.
diff --git a/nscd/connections.c b/nscd/connections.c
index f463f45..180ae77 100644
--- a/nscd/connections.c
+++ b/nscd/connections.c
@@ -649,8 +649,8 @@ cannot create read-only descriptor for \"%s\"; no mmap"),
 		  close (fd);
 	      }
 	    else if (errno == EACCES)
-	      error (EXIT_FAILURE, 0, _("cannot access '%s'"),
-		     dbs[cnt].db_filename);
+	      do_exit (EXIT_FAILURE, 0, _("cannot access '%s'"),
+		       dbs[cnt].db_filename);
 	  }
 
 	if (dbs[cnt].head == NULL)
@@ -699,8 +699,7 @@ cannot create read-only descriptor for \"%s\"; no mmap"),
 		  {
 		    dbg_log (_("database for %s corrupted or simultaneously used; remove %s manually if necessary and restart"),
 			     dbnames[cnt], dbs[cnt].db_filename);
-		    // XXX Correct way to terminate?
-		    exit (1);
+		    do_exit (1, 0, NULL);
 		  }
 
 		if  (dbs[cnt].persistent)
@@ -867,7 +866,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
   if (sock < 0)
     {
       dbg_log (_("cannot open socket: %s"), strerror (errno));
-      exit (errno == EACCES ? 4 : 1);
+      do_exit (errno == EACCES ? 4 : 1, 0, NULL);
     }
   /* Bind a name to the socket.  */
   struct sockaddr_un sock_addr;
@@ -876,7 +875,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
   if (bind (sock, (struct sockaddr *) &sock_addr, sizeof (sock_addr)) < 0)
     {
       dbg_log ("%s: %s", _PATH_NSCDSOCKET, strerror (errno));
-      exit (errno == EACCES ? 4 : 1);
+      do_exit (errno == EACCES ? 4 : 1, 0, NULL);
     }
 
 #ifndef __ASSUME_SOCK_CLOEXEC
@@ -888,7 +887,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
 	{
 	  dbg_log (_("cannot change socket to nonblocking mode: %s"),
 		   strerror (errno));
-	  exit (1);
+	  do_exit (1, 0, NULL);
 	}
 
       /* The descriptor needs to be closed on exec.  */
@@ -896,7 +895,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
 	{
 	  dbg_log (_("cannot set socket to close on exec: %s"),
 		   strerror (errno));
-	  exit (1);
+	  do_exit (1, 0, NULL);
 	}
     }
 #endif
@@ -909,7 +908,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
     {
       dbg_log (_("cannot enable socket to accept connections: %s"),
 	       strerror (errno));
-      exit (1);
+      do_exit (1, 0, NULL);
     }
 
 #ifdef HAVE_NETLINK
@@ -953,7 +952,7 @@ cannot set socket to close on exec: %s; disabling paranoia mode"),
 		      dbg_log (_("\
 cannot change socket to nonblocking mode: %s"),
 			       strerror (errno));
-		      exit (1);
+		      do_exit (1, 0, NULL);
 		    }
 
 		  /* The descriptor needs to be closed on exec.  */
@@ -962,7 +961,7 @@ cannot change socket to nonblocking mode: %s"),
 		    {
 		      dbg_log (_("cannot set socket to close on exec: %s"),
 			       strerror (errno));
-		      exit (1);
+		      do_exit (1, 0, NULL);
 		    }
 		}
 # endif
@@ -2392,7 +2391,7 @@ start_threads (void)
       if (pthread_cond_init (&dbs[i].prune_cond, &condattr) != 0)
 	{
 	  dbg_log (_("could not initialize conditional variable"));
-	  exit (1);
+	  do_exit (1, 0, NULL);
 	}
 
       pthread_t th;
@@ -2400,7 +2399,7 @@ start_threads (void)
 	  && pthread_create (&th, &attr, nscd_run_prune, (void *) i) != 0)
 	{
 	  dbg_log (_("could not start clean-up thread; terminating"));
-	  exit (1);
+	  do_exit (1, 0, NULL);
 	}
     }
 
@@ -2414,13 +2413,17 @@ start_threads (void)
 	  if (i == 0)
 	    {
 	      dbg_log (_("could not start any worker thread; terminating"));
-	      exit (1);
+	      do_exit (1, 0, NULL);
 	    }
 
 	  break;
 	}
     }
 
+  /* Now it is safe to let the parent know that we're doing fine and it can
+     exit.  */
+  notify_parent (0);
+
   /* Determine how much room for descriptors we should initially
      allocate.  This might need to change later if we cap the number
      with MAXCONN.  */
@@ -2465,8 +2468,8 @@ begin_drop_privileges (void)
   if (pwd == NULL)
     {
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      error (EXIT_FAILURE, 0, _("Failed to run nscd as user '%s'"),
-	     server_user);
+      do_exit (EXIT_FAILURE, 0,
+	       _("Failed to run nscd as user '%s'"), server_user);
     }
 
   server_uid = pwd->pw_uid;
@@ -2483,7 +2486,8 @@ begin_drop_privileges (void)
     {
       /* This really must never happen.  */
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      error (EXIT_FAILURE, errno, _("initial getgrouplist failed"));
+      do_exit (EXIT_FAILURE, errno,
+	       _("initial getgrouplist failed"));
     }
 
   server_groups = (gid_t *) xmalloc (server_ngroups * sizeof (gid_t));
@@ -2492,7 +2496,7 @@ begin_drop_privileges (void)
       == -1)
     {
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      error (EXIT_FAILURE, errno, _("getgrouplist failed"));
+      do_exit (EXIT_FAILURE, errno, _("getgrouplist failed"));
     }
 }
 
@@ -2510,7 +2514,7 @@ finish_drop_privileges (void)
   if (setgroups (server_ngroups, server_groups) == -1)
     {
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      error (EXIT_FAILURE, errno, _("setgroups failed"));
+      do_exit (EXIT_FAILURE, errno, _("setgroups failed"));
     }
 
   int res;
@@ -2521,8 +2525,7 @@ finish_drop_privileges (void)
   if (res == -1)
     {
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      perror ("setgid");
-      exit (4);
+      do_exit (4, errno, "setgid");
     }
 
   if (paranoia)
@@ -2532,8 +2535,7 @@ finish_drop_privileges (void)
   if (res == -1)
     {
       dbg_log (_("Failed to run nscd as user '%s'"), server_user);
-      perror ("setuid");
-      exit (4);
+      do_exit (4, errno, "setuid");
     }
 
 #if defined HAVE_LIBAUDIT && defined HAVE_LIBCAP
diff --git a/nscd/nscd.c b/nscd/nscd.c
index 63d9d83..5680378 100644
--- a/nscd/nscd.c
+++ b/nscd/nscd.c
@@ -39,6 +39,8 @@
 #include <sys/stat.h>
 #include <sys/uio.h>
 #include <sys/un.h>
+#include <sys/wait.h>
+#include <stdarg.h>
 
 #include "dbg_log.h"
 #include "nscd.h"
@@ -101,6 +103,7 @@ gid_t old_gid;
 
 static int check_pid (const char *file);
 static int write_pid (const char *file);
+static int monitor_child (int fd);
 
 /* Name and version of program.  */
 static void print_version (FILE *stream, struct argp_state *state);
@@ -142,6 +145,7 @@ static struct argp argp =
 
 /* True if only statistics are requested.  */
 static bool get_stats;
+static int parent_fd = -1;
 
 int
 main (int argc, char **argv)
@@ -196,11 +200,27 @@ main (int argc, char **argv)
       /* Behave like a daemon.  */
       if (run_mode == RUN_DAEMONIZE)
 	{
+	  int fd[2];
+
+	  if (pipe (fd) != 0)
+	    error (EXIT_FAILURE, errno,
+		   _("cannot create a pipe to talk to the child"));
+
 	  pid = fork ();
 	  if (pid == -1)
 	    error (EXIT_FAILURE, errno, _("cannot fork"));
 	  if (pid != 0)
-	    exit (0);
+	    {
+	      /* The parent only reads from the child.  */
+	      close (fd[1]);
+	      exit (monitor_child (fd[0]));
+	    }
+	  else
+	    {
+	      /* The child only writes to the parent.  */
+	      close (fd[0]);
+	      parent_fd = fd[1];
+	    }
 	}
 
       int nullfd = open (_PATH_DEVNULL, O_RDWR);
@@ -242,7 +262,8 @@ main (int argc, char **argv)
 	      char *endp;
 	      long int fdn = strtol (dirent->d_name, &endp, 10);
 
-	      if (*endp == '\0' && fdn != dfdn && fdn >= min_close_fd)
+	      if (*endp == '\0' && fdn != dfdn && fdn >= min_close_fd
+		  && fdn != parent_fd)
 		close ((int) fdn);
 	    }
 
@@ -250,13 +271,14 @@ main (int argc, char **argv)
 	}
       else
 	for (i = min_close_fd; i < getdtablesize (); i++)
-	  close (i);
+	  if (i != parent_fd)
+	    close (i);
 
       setsid ();
 
       if (chdir ("/") != 0)
-	error (EXIT_FAILURE, errno,
-	       _("cannot change current working directory to \"/\""));
+	do_exit (EXIT_FAILURE, errno,
+		 _("cannot change current working directory to \"/\""));
 
       openlog ("nscd", LOG_CONS | LOG_ODELAY, LOG_DAEMON);
 
@@ -592,3 +614,79 @@ write_pid (const char *file)
 
   return result;
 }
+
+static int
+monitor_child (int fd)
+{
+  int child_ret = 0;
+  int ret = read (fd, &child_ret, sizeof (child_ret));
+
+  /* The child terminated with an error, either via exit or some other abnormal
+     method, like a segfault.  */
+  if (ret <= 0 || child_ret != 0)
+    {
+      int err = wait (&child_ret);
+
+      if (err < 0)
+	{
+	  fprintf (stderr, _("wait failed"));
+	  return 1;
+	}
+
+      fprintf (stderr, _("child exited with status %d"),
+	       WEXITSTATUS (child_ret));
+      if (WIFSIGNALED (child_ret))
+	fprintf (stderr, _(", terminated by signal %d.\n"),
+		 WTERMSIG (child_ret));
+      else
+	fprintf (stderr, ".\n");
+    }
+
+  /* We have the child status, so exit with that code.  */
+  close (fd);
+
+  return child_ret;
+}
+
+void
+do_exit (int child_ret, int errnum, const char *format, ...)
+{
+  if (parent_fd != -1)
+    {
+      int ret = write (parent_fd, &child_ret, sizeof (child_ret));
+      assert (ret == sizeof (child_ret));
+      close (parent_fd);
+    }
+
+  if (format != NULL)
+    {
+      /* Emulate error() since we don't have a va_list variant for it.  */
+      va_list argp;
+
+      fflush (stdout);
+
+      fprintf (stderr, "%s: ", program_invocation_name);
+
+      va_start (argp, format);
+      vfprintf (stderr, format, argp);
+      va_end (argp);
+
+      fprintf (stderr, ": %s\n", strerror (errnum));
+      fflush (stderr);
+    }
+
+  /* Finally, exit.  */
+  exit (child_ret);
+}
+
+void
+notify_parent (int child_ret)
+{
+  if (parent_fd == -1)
+    return;
+
+  int ret = write (parent_fd, &child_ret, sizeof (child_ret));
+  assert (ret == sizeof (child_ret));
+  close (parent_fd);
+  parent_fd = -1;
+}
diff --git a/nscd/nscd.h b/nscd/nscd.h
index 972f462..529b3f5 100644
--- a/nscd/nscd.h
+++ b/nscd/nscd.h
@@ -205,6 +205,8 @@ extern gid_t old_gid;
 /* nscd.c */
 extern void termination_handler (int signum) __attribute__ ((__noreturn__));
 extern int nscd_open_socket (void);
+void notify_parent (int child_ret);
+void do_exit (int child_ret, int errnum, const char *format, ...);
 
 /* connections.c */
 extern void nscd_init (void);
diff --git a/nscd/selinux.c b/nscd/selinux.c
index e477254..46b0ea9 100644
--- a/nscd/selinux.c
+++ b/nscd/selinux.c
@@ -179,7 +179,7 @@ preserve_capabilities (void)
   if (prctl (PR_SET_KEEPCAPS, 1) == -1)
     {
       dbg_log (_("Failed to set keep-capabilities"));
-      error (EXIT_FAILURE, errno, _("prctl(KEEPCAPS) failed"));
+      do_exit (EXIT_FAILURE, errno, _("prctl(KEEPCAPS) failed"));
       /* NOTREACHED */
     }
 
@@ -194,7 +194,7 @@ preserve_capabilities (void)
 	cap_free (tmp_caps);
 
       dbg_log (_("Failed to initialize drop of capabilities"));
-      error (EXIT_FAILURE, 0, _("cap_init failed"));
+      do_exit (EXIT_FAILURE, 0, _("cap_init failed"));
     }
 
   /* There is no reason why these should not work.  */
@@ -216,7 +216,7 @@ preserve_capabilities (void)
     {
       cap_free (new_caps);
       dbg_log (_("Failed to drop capabilities"));
-      error (EXIT_FAILURE, 0, _("cap_set_proc failed"));
+      do_exit (EXIT_FAILURE, 0, _("cap_set_proc failed"));
     }
 
   return new_caps;
@@ -233,7 +233,7 @@ install_real_capabilities (cap_t new_caps)
     {
       cap_free (new_caps);
       dbg_log (_("Failed to drop capabilities"));
-      error (EXIT_FAILURE, 0, _("cap_set_proc failed"));
+      do_exit (EXIT_FAILURE, 0, _("cap_set_proc failed"));
       /* NOTREACHED */
     }
 
@@ -242,7 +242,7 @@ install_real_capabilities (cap_t new_caps)
   if (prctl (PR_SET_KEEPCAPS, 0) == -1)
     {
       dbg_log (_("Failed to unset keep-capabilities"));
-      error (EXIT_FAILURE, errno, _("prctl(KEEPCAPS) failed"));
+      do_exit (EXIT_FAILURE, errno, _("prctl(KEEPCAPS) failed"));
       /* NOTREACHED */
     }
 }
@@ -258,7 +258,7 @@ nscd_selinux_enabled (int *selinux_enabled)
   if (*selinux_enabled < 0)
     {
       dbg_log (_("Failed to determine if kernel supports SELinux"));
-      exit (EXIT_FAILURE);
+      do_exit (EXIT_FAILURE, 0, NULL);
     }
 }
 
@@ -272,7 +272,7 @@ avc_create_thread (void (*run) (void))
   rc =
     pthread_create (&avc_notify_thread, NULL, (void *(*) (void *)) run, NULL);
   if (rc != 0)
-    error (EXIT_FAILURE, rc, _("Failed to start AVC thread"));
+    do_exit (EXIT_FAILURE, rc, _("Failed to start AVC thread"));
 
   return &avc_notify_thread;
 }
@@ -294,7 +294,7 @@ avc_alloc_lock (void)
 
   avc_mutex = malloc (sizeof (pthread_mutex_t));
   if (avc_mutex == NULL)
-    error (EXIT_FAILURE, errno, _("Failed to create AVC lock"));
+    do_exit (EXIT_FAILURE, errno, _("Failed to create AVC lock"));
   pthread_mutex_init (avc_mutex, NULL);
 
   return avc_mutex;
@@ -334,7 +334,7 @@ nscd_avc_init (void)
   avc_entry_ref_init (&aeref);
 
   if (avc_init ("avc", NULL, &log_cb, &thread_cb, &lock_cb) < 0)
-    error (EXIT_FAILURE, errno, _("Failed to start AVC"));
+    do_exit (EXIT_FAILURE, errno, _("Failed to start AVC"));
   else
     dbg_log (_("Access Vector Cache (AVC) started"));
 #ifdef HAVE_LIBAUDIT

Attachment: pgp3IRdWQaUtW.pgp
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]