global: do not start two daemons with a single pid-file (part 2)

Fixes the following bugs:

* the fd is open(O_WRONLY) and cannot be read from, safe_read
  always fails and never removes the pid file.

* pidfile_open(g_conf) is close(STDOUT_FILENO) and there is a risk that
  pidfile_open gets STDOUT_FILENO only to have it closed and redirected
  to /dev/null.

* Before writing the file, ftruncate it so that overriding a file
  containing the pid 1234 with the pid 89 does not end up being
  a file with 8934.

* Before reading the file, lseek back to offset 0 otherwise it
  will read nothing.

* tests_pidfile was missing an argument when failing
  TEST_without_pidfile and killed all process with ceph in their name,
  leading to chaos and no useful error message.

* lstat(fd) cannot possibly return a result different from the one
  obtained right after the file was open, stat(path) must be used
  instead.

In addition to fixing the bugs above, refactor the pidfile.cc
implementation to:

* be systematic about error reporting (using cerr for when removing
  the pidfile because derr is not available at this point and derr
  when creating the pidfile).

* replace pidfile_open / pidfile_write with just pidfile_write since
  there never is a case when they are not used together.

More test cases are added to test_pidfile to verify the bugs above are
fixed.

http://tracker.ceph.com/issues/13422 Fixes: #13422

Signed-off-by: Loic Dachary <loic@dachary.org>
(cherry picked from commit 9828d49d6f3ccfc78d496153d263ea39b1722d4b)

Conflicts:
	src/global/global_init.cc
      - the `flag` argument of `global_init_prefork()` is not used, so
        it was removed in master. but the cleanup commit was not
        cherry-picked to hammer, thus the conflict. we can just keep it
        around in hammer to minimize the code churn, although it may
        stand in the way of future backports.)
      - s/nullptr/NULL/ as hammer does not support c++11.

diff --git a/src/global/global_init.cc b/src/global/global_init.cc
index 6be6b2e717e580af798779c61b1cf65e7008e5ee..1c34d44728583e8dc1b086ccf8eba020fe60717b 100644 (file)
--- a/src/global/global_init.cc
+++ b/src/global/global_init.cc
@@ -151,12 +151,7 @@ void global_print_banner(void)
   output_ceph_version();
 }
 
-static void pidfile_remove_void(void)
-{
-  pidfile_remove();
-}
-
-int global_init_prefork(CephContext *cct, int flags)
+int global_init_prefork(CephContext *cct, int)
 {
   if (g_code_env != CODE_ENVIRONMENT_DAEMON)
     return -1;
@@ -164,15 +159,8 @@ int global_init_prefork(CephContext *cct, int flags)
   const md_config_t *conf = cct->_conf;
   if (!conf->daemonize) {
 
-    if (pidfile_open(g_conf) < 0) {
+    if (pidfile_write(g_conf) < 0)
       exit(1);
-    }
-
-    if (atexit(pidfile_remove_void)) {
-      derr << "global_init_daemonize: failed to set pidfile_remove function "
-          << "to run at exit." << dendl;
-    }
-    pidfile_write();
 
     return -1;
   }
@@ -205,15 +193,6 @@ void global_init_postfork_start(CephContext *cct)
   // restart log thread
   g_ceph_context->_log->start();
 
-  if (pidfile_open(g_conf) < 0) {
-    exit(1);
-  }
-
-  if (atexit(pidfile_remove_void)) {
-    derr << "global_init_daemonize: failed to set pidfile_remove function "
-        << "to run at exit." << dendl;
-  }
-
   /* This is the old trick where we make file descriptors 0, 1, and possibly 2
    * point to /dev/null.
    *
@@ -237,7 +216,8 @@ void global_init_postfork_start(CephContext *cct)
     exit(1);
   }
 
-  pidfile_write();
+  if (pidfile_write(g_conf) < 0)
+    exit(1);
 }
 
 void global_init_postfork_finish(CephContext *cct, int flags)

diff --git a/src/global/pidfile.cc b/src/global/pidfile.cc
index c9452647ae99bc25c7b8916282902c2a5e64c6a9..f97999f40e0882008b3e5fa4125ab005857109ba 100644 (file)
--- a/src/global/pidfile.cc
+++ b/src/global/pidfile.cc
@@ -29,6 +29,13 @@
 
 #include "include/compat.h"
 
+//
+// derr can be used for functions exclusively called from pidfile_write
+//
+// cerr must be used for functions called by pidfile_remove because
+// logging is not functional when it is called. cerr output is lost
+// when the caller is daemonized but it will show if not (-f)
+//
 #define dout_prefix *_dout
 
 struct pidfh {
@@ -36,139 +43,189 @@ struct pidfh {
   char pf_path[PATH_MAX + 1];
   dev_t pf_dev;
   ino_t pf_ino;
- 
-  pidfh() : pf_fd(-1), pf_dev(0), pf_ino(0) {
-    memset(pf_path, 0, sizeof(pf_path));
+
+  pidfh() {
+    reset();
+  }
+  ~pidfh() {
+    remove();
+  }
+
+  bool is_open() {
+    return pf_path[0] != '\0' && pf_fd != -1;
   }
-  
-  void close() {
+  void reset() {
     pf_fd = -1;
-    pf_path[0] = '\0';
+    memset(pf_path, 0, sizeof(pf_path));
     pf_dev = 0;
     pf_ino = 0;
   }
+  int verify();
+  int remove();
+  int open(const md_config_t *conf);
+  int write();
 };
-static pidfh pfh;
 
-static int pidfile_verify() {
-  struct stat st;
+static pidfh *pfh = NULL;
 
-  if (pfh.pf_fd == -1) {
+int pidfh::verify() {
+  // check that the file we opened still is the same
+  if (pf_fd == -1)
     return -EINVAL;
-  }
-  /*
-   * Check remembered descriptor
-   */ 
-  if (fstat(pfh.pf_fd, &st) == -1) {
+  struct stat st;
+  if (stat(pf_path, &st) == -1)
     return -errno;
-  }
-
-  if (st.st_dev != pfh.pf_dev || st.st_ino != pfh.pf_ino) {
+  if (st.st_dev != pf_dev || st.st_ino != pf_ino)
     return -ESTALE;
-  }
   return 0;
 }
 
-int pidfile_write() 
+int pidfh::remove()
 {
-  if (!pfh.pf_path[0]) {
+  if (!pf_path[0])
     return 0;
-  }
 
   int ret;
-  if ((ret = pidfile_verify()) < 0) {
-    return ret;
-  }
-  
-  char buf[32];
-  int len = snprintf(buf, sizeof(buf), "%d\n", getpid());
-  ret = safe_write(pfh.pf_fd, buf, len);
-  if (ret < 0) {
-    derr << "write_pid_file: failed to write to pid file '"
-        << pfh.pf_path << "': " << cpp_strerror(ret) << dendl;
-    pfh.close(); 
+  if ((ret = verify()) < 0) {
+    if (pf_fd != -1) {
+      ::close(pf_fd);
+      reset();
+    }
     return ret;
   }
 
-  return 0;
-}
-
-int pidfile_remove()
-{
-  if (!pfh.pf_path[0]) {
-    return 0;
-  }
-
-  int ret;
-  if ((ret = pidfile_verify()) < 0) {
-    if (pfh.pf_fd != -1) {
-      ::close(pfh.pf_fd);
-    }
-    return ret;
+  // seek to the beginning of the file before reading
+  ret = ::lseek(pf_fd, 0, SEEK_SET);
+  if (ret < 0) {
+    std::cerr << __func__ << " lseek failed "
+             << cpp_strerror(errno) << std::endl;
+    return -errno;
   }
 
-  // only remove it if it has OUR pid in it!
+  // check that the pid file still has our pid in it
   char buf[32];
   memset(buf, 0, sizeof(buf));
-  ssize_t res = safe_read(pfh.pf_fd, buf, sizeof(buf));
-  if (pfh.pf_fd != -1) {
-    ::close(pfh.pf_fd);
-  }
-
+  ssize_t res = safe_read(pf_fd, buf, sizeof(buf));
+  ::close(pf_fd);
   if (res < 0) {
+    std::cerr << __func__ << " safe_read failed "
+             << cpp_strerror(-res) << std::endl;
     return res;
   }
-  
+
   int a = atoi(buf);
   if (a != getpid()) {
+    std::cerr << __func__ << " the pid found in the file is "
+             << a << " which is different from getpid() "
+             << getpid() << std::endl;
     return -EDOM;
   }
-  res = ::unlink(pfh.pf_path);
-  if (res) {
-    return res;
+  ret = ::unlink(pf_path);
+  if (ret < 0) {
+    std::cerr << __func__ << " unlink " << pf_path << " failed "
+             << cpp_strerror(errno) << std::endl;
+    return -errno;
   }
-  pfh.close();
+  reset();
   return 0;
 }
 
-int pidfile_open(const md_config_t *conf) 
+int pidfh::open(const md_config_t *conf)
 {
-  if (conf->pid_file.empty()) {
-    return 0;
-  }
-  int len = snprintf(pfh.pf_path, sizeof(pfh.pf_path),
-                    "%s", conf->pid_file.c_str());
+  int len = snprintf(pf_path, sizeof(pf_path),
+                   "%s", conf->pid_file.c_str());
 
-  if (len >= (int)sizeof(pfh.pf_path)) { 
+  if (len >= (int)sizeof(pf_path))
     return -ENAMETOOLONG;
-  }
 
   int fd;
-  fd = ::open(pfh.pf_path, O_CREAT|O_WRONLY, 0644);
+  fd = ::open(pf_path, O_CREAT|O_RDWR, 0644);
   if (fd < 0) {
     int err = errno;
-    derr << "write_pid_file: failed to open pid file '"
-         << pfh.pf_path << "': " << cpp_strerror(err) << dendl;
-    pfh.close();
-    return -errno;
+    derr << __func__ << ": failed to open pid file '"
+        << pf_path << "': " << cpp_strerror(err) << dendl;
+    reset();
+    return -err;
   }
-  struct stat st;   
+  struct stat st;
   if (fstat(fd, &st) == -1) {
-    close(fd);
-    pfh.close();  
-    return -errno;
+    int err = errno;
+    derr << __func__ << ": failed to fstat pid file '"
+        << pf_path << "': " << cpp_strerror(err) << dendl;
+    ::close(fd);
+    reset();
+    return -err;
   }
-  pfh.pf_fd = fd;
-  pfh.pf_dev = st.st_dev;
-  pfh.pf_ino = st.st_ino;
-  
+
+  pf_fd = fd;
+  pf_dev = st.st_dev;
+  pf_ino = st.st_ino;
+
   struct flock l = { F_WRLCK, SEEK_SET, 0, 0, 0 };
-  int r = ::fcntl(pfh.pf_fd, F_SETLK, &l);
+  int r = ::fcntl(pf_fd, F_SETLK, &l);
   if (r < 0) {
-    derr << "failed to lock pidfile - " << pfh.pf_path << ". is there another process in use?" << dendl;
-    close(pfh.pf_fd);
-    pfh.close();
+    derr << __func__ << ": failed to lock pidfile "
+        << pf_path << " because another process locked it." << dendl;
+    ::close(pf_fd);
+    reset();
     return -errno;
   }
   return 0;
 }
+
+int pidfh::write()
+{
+  if (!is_open())
+    return 0;
+
+  char buf[32];
+  int len = snprintf(buf, sizeof(buf), "%d\n", getpid());
+  if (::ftruncate(pf_fd, 0) < 0) {
+    int err = errno;
+    derr << __func__ << ": failed to ftruncate the pid file '"
+        << pf_path << "': " << cpp_strerror(err) << dendl;
+    return err;
+  }
+  ssize_t res = safe_write(pf_fd, buf, len);
+  if (res < 0) {
+    derr << __func__ << ": failed to write to pid file '"
+        << pf_path << "': " << cpp_strerror(-res) << dendl;
+    return res;
+  }
+  return 0;
+}
+
+void pidfile_remove()
+{
+  delete pfh;
+  pfh = NULL;
+}
+
+int pidfile_write(const md_config_t *conf)
+{
+  if (conf->pid_file.empty())
+    return 0;
+
+  assert(!pfh);
+
+  pfh = new pidfh();
+  if (atexit(pidfile_remove)) {
+    derr << __func__ << ": failed to set pidfile_remove function "
+        << "to run at exit." << dendl;
+    return -EINVAL;
+  }
+
+  int r = pfh->open(conf);
+  if (r != 0) {
+    pidfile_remove();
+    return r;
+  }
+
+  r = pfh->write();
+  if (r != 0) {
+    pidfile_remove();
+    return r;
+  }
+
+  return 0;
+}

diff --git a/src/global/pidfile.h b/src/global/pidfile.h
index 49ea5416ac1488b31789db7c4b041830d3f3570b..e7e2b0d4edd10bd322b847006189d2e44e2f5cff 100644 (file)
--- a/src/global/pidfile.h
+++ b/src/global/pidfile.h
@@ -19,13 +19,10 @@ struct md_config_t;
 
 // Write a pidfile with the current pid, using the configuration in the
 // provided conf structure.
-int pidfile_write();
+int pidfile_write(const md_config_t *conf);
 
 // Remove the pid file that was previously written by pidfile_write.
 // This is safe to call in a signal handler context.
-int pidfile_remove();
-
-// test whether the pid_file is being used by another process
-int pidfile_open(const md_config_t *conf);
+void pidfile_remove();
 
 #endif