From: Loic Dachary <ldachary@redhat.com>
Date: Mon, 1 Feb 2016 12:32:13 +0000 (+0700)
Subject: global: do not start two daemons with a single pid-file (part 2)
X-Git-Tag: v10.0.4~31^2
X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=9828d49d6f3ccfc78d496153d263ea39b1722d4b;p=ceph.git

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>
---

diff --git a/src/global/global_init.cc b/src/global/global_init.cc
index e721abae5ecc..c5feb744924f 100644
--- a/src/global/global_init.cc
+++ b/src/global/global_init.cc
@@ -252,11 +252,6 @@ void global_print_banner(void)
   output_ceph_version();
 }
 
-static void pidfile_remove_void(void)
-{
-  pidfile_remove();
-}
-
 int global_init_prefork(CephContext *cct)
 {
   if (g_code_env != CODE_ENVIRONMENT_DAEMON)
@@ -265,15 +260,8 @@ int global_init_prefork(CephContext *cct)
   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;
   }
@@ -310,15 +298,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.
    *
@@ -342,7 +321,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)
diff --git a/src/global/pidfile.cc b/src/global/pidfile.cc
index c9452647ae99..17fe6800cc0f 100644
--- 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,190 @@ 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 = nullptr;
 
-  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()
+{
+  if (pfh != nullptr)
+    delete pfh;
+  pfh = nullptr;
+}
+
+int pidfile_write(const md_config_t *conf)
+{
+  if (conf->pid_file.empty())
+    return 0;
+
+  assert(pfh == nullptr);
+
+  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 49ea5416ac14..e7e2b0d4edd1 100644
--- 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
diff --git a/src/test/test_pidfile.sh b/src/test/test_pidfile.sh
index 2a5cb27c654e..2dec1f74a0ff 100755
--- a/src/test/test_pidfile.sh
+++ b/src/test/test_pidfile.sh
@@ -11,7 +11,7 @@ function run() {
     local dir=$1
     shift
 
-    export CEPH_MON="127.0.0.1:17108"
+    export CEPH_MON="127.0.0.1:7124" # git grep '\<7124\>' : there must be only one
     export CEPH_ARGS
     CEPH_ARGS+="--fsid=$(uuidgen) --auth-supported=none "
     CEPH_ARGS+="--mon-host=$CEPH_MON "
@@ -26,8 +26,8 @@ function TEST_without_pidfile() {
     local dir=$1
     setup $dir
     local RUNID=`uuidgen`
-    run_mon $dir a --pid-file= --daemonize=$RUNID || { teardown_unexist_pidfile $dir; return 1; } 
-    run_osd $dir 0 --pid-file= --daemonize=$RUNID || { teardown_unexist_pidfile $dir; return 1; }
+    run_mon $dir a --pid-file= --daemonize=$RUNID || { teardown_unexist_pidfile $dir $RUNID; return 1; }
+    run_osd $dir 0 --pid-file= --daemonize=$RUNID || { teardown_unexist_pidfile $dir $RUNID; return 1; }
     teardown_unexist_pidfile $dir $RUNID || return 1
 }
 
@@ -61,15 +61,41 @@ function teardown_unexist_pidfile() {
     return $status
 }
 
-function TEST_contend_pidfile() {
+function TEST_pidfile() {
     local dir=$1
     setup $dir 
- 
-    run_mon $dir a 
+
+    # no daemon can use a pidfile that is owned by another daemon
+    run_mon $dir a || return 1
     run_mon $dir a 2>&1 | grep "failed to lock pidfile" || return 1
 
-    run_osd $dir 0 
+    run_osd $dir 0 || return 1
     run_osd $dir 0 2>&1 | grep "failed to lock pidfile" || return 1
+
+    # when a daemon shutdown, it will not unlink a path different from
+    # the one it owns
+    mv $dir/osd.0.pid $dir/osd.0.pid.old || return 1
+    cp $dir/osd.0.pid.old $dir/osd.0.pid || return 1
+    kill_daemons $dir TERM osd.0 || return 1
+    test -f $dir/osd.0.pid || return 1
+
+    # when a daemon starts, it re-uses the pid file if no other daemon
+    # has it locked
+    run_osd $dir 0 || return 1
+    ! cmp $dir/osd.0.pid $dir/osd.0.pid.old || return 1
+
+    # if the pid in the file is different from the pid of the daemon
+    # the file is not removed because it is assumed to be owned by
+    # another daemon
+    echo 123 > $dir/osd.0.pid
+    kill_daemons $dir TERM osd.0 || return 1
+    test -f $dir/osd.0.pid || return 1
+
+    # when the daemon shutdown, it removes its own pid file
+    test -f $dir/mon.a.pid || return 1
+    kill_daemons $dir TERM mon.a || return 1
+    ! test -f $dir/mon.a.pid || return 1
+
     teardown $dir || return 1
 }