mon: convert store users with unchecked return codes to just assert on issues

This will make them much more noticeable and reduce the odds of something
writing data which assumes the previous op succeeded.

Signed-off-by: Greg Farnum <greg@inktank.com>

diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc
index 14fe963cec6f9a13705ba61a7253b6202ced4de9..5edecfc14c8e1165faa94c6863feb2e399dcc9d8 100644 (file)
--- a/src/mon/Monitor.cc
+++ b/src/mon/Monitor.cc
@@ -2360,7 +2360,8 @@ int Monitor::write_fsid()
 
   bufferlist b;
   b.append(us);
-  return store->put_bl_ss(b, "cluster_uuid", 0);
+  store->put_bl_ss(b, "cluster_uuid", 0);
+  return 0;
 }
 
 /*
@@ -2384,9 +2385,8 @@ int Monitor::mkfs(bufferlist& osdmapbl)
   bufferlist magicbl;
   magicbl.append(CEPH_MON_ONDISK_MAGIC);
   magicbl.append("\n");
-  r = store->put_bl_ss(magicbl, "magic", 0);
-  if (r < 0)
-    return r;
+  store->put_bl_ss(magicbl, "magic", 0);
+
 
   features = get_supported_features();
   write_features();

diff --git a/src/mon/MonitorStore.cc b/src/mon/MonitorStore.cc
index 85db3204da7506f580c5bf08a567385004879ae6..0b7bc3a93876cb1c919ed0ce642fc2854e90f90d 100644 (file)
--- a/src/mon/MonitorStore.cc
+++ b/src/mon/MonitorStore.cc
@@ -35,7 +35,6 @@ static ostream& _prefix(std::ostream *_dout, const string& dir) {
   return *_dout << "store(" << dir << ") ";
 }
 
-
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -136,6 +135,7 @@ version_t MonitorStore::get_int(const char *a, const char *b)
     }
     derr << "MonitorStore::get_int: failed to open '" << fn << "': "
         << cpp_strerror(err) << dendl;
+    assert(0 == "failed to open");
     return 0;
   }
   
@@ -147,6 +147,7 @@ version_t MonitorStore::get_int(const char *a, const char *b)
         << cpp_strerror(r) << dendl;
     int close_err = TEMP_FAILURE_RETRY(::close(fd));
     assert(0 == close_err);
+    assert(0); // the file exists; so this is a different failure
     return 0;
   }
   int close_err = TEMP_FAILURE_RETRY(::close(fd));
@@ -260,7 +261,7 @@ bool MonitorStore::exists_bl_ss(const char *a, const char *b)
   return r == 0;
 }
 
-int MonitorStore::erase_ss(const char *a, const char *b)
+void MonitorStore::erase_ss(const char *a, const char *b)
 {
   char fn[1024];
   char dr[1024];
@@ -273,6 +274,7 @@ int MonitorStore::erase_ss(const char *a, const char *b)
     strcpy(fn, dr);
   }
   int r = ::unlink(fn);
+  assert(0 == r || ENOENT == errno); // callers don't check for existence first
 
   if (b) {
     // wipe out _gv file too, if any.  this is sloppy, but will work.
@@ -282,7 +284,6 @@ int MonitorStore::erase_ss(const char *a, const char *b)
   }
 
   ::rmdir(dr);  // sloppy attempt to clean up empty dirs
-  return r;
 }
 
 int MonitorStore::get_bl_ss(bufferlist& bl, const char *a, const char *b)
@@ -338,7 +339,7 @@ int MonitorStore::get_bl_ss(bufferlist& bl, const char *a, const char *b)
   return len;
 }
 
-int MonitorStore::write_bl_ss_impl(bufferlist& bl, const char *a, const char *b, bool append)
+void MonitorStore::write_bl_ss_impl(bufferlist& bl, const char *a, const char *b, bool append)
 {
   int err = 0;
   char fn[1024];
@@ -349,7 +350,7 @@ int MonitorStore::write_bl_ss_impl(bufferlist& bl, const char *a, const char *b,
       err = -errno;
       derr << __func__ << " failed to create dir " << fn
           << ": " << cpp_strerror(err) << dendl;
-      return err;
+      assert(0 == "failed to create dir");
     }
     dout(15) << "put_bl " << a << "/" << b << " = " << bl.length() << " bytes" << dendl;
     snprintf(fn, sizeof(fn), "%s/%s/%s", dir.c_str(), a, b);
@@ -365,7 +366,7 @@ int MonitorStore::write_bl_ss_impl(bufferlist& bl, const char *a, const char *b,
       err = -errno;
       derr << "failed to open " << fn << "for append: "
           << cpp_strerror(err) << dendl;
-      return err;
+      assert(0 == "failed to open for append");
     }
   } else {
     snprintf(tfn, sizeof(tfn), "%s.new", fn);
@@ -373,42 +374,33 @@ int MonitorStore::write_bl_ss_impl(bufferlist& bl, const char *a, const char *b,
     if (fd < 0) {
       err = -errno;
       derr << "failed to open " << tfn << ": " << cpp_strerror(err) << dendl;
-      return err;
+      assert(0 == "failed to open");
     }
   }
   
   err = bl.write_fd(fd);
-
-  if (!err)
-    err = ::fsync(fd);
-  int close_err = TEMP_FAILURE_RETRY(::close(fd));
-  assert (0 == close_err); // this really can't fail, right? right?...
-  if (!append && !err) {
-    int r = ::rename(tfn, fn);
-    if (r < 0) {
+  assert(!err);
+  err = ::fsync(fd);
+  assert(!err);
+  err = TEMP_FAILURE_RETRY(::close(fd));
+  assert (!err); // this really can't fail, right? right?...
+  if (!append) {
+    err = ::rename(tfn, fn);
+    if (err < 0) {
       err = -errno;
       derr << __func__ << " failed to rename '" << tfn << "' -> '"
           << fn << "': " << cpp_strerror(err) << dendl;
-      return err;
+      assert(0 == "failed to rename");
     }
-  } else if (err) {
-    err = -errno;
-    derr << __func__ << "failed to write or fsync " << fn  << cpp_strerror(err) << dendl;
   }
-
-  return err;
 }
 
-int MonitorStore::write_bl_ss(bufferlist& bl, const char *a, const char *b, bool append)
+void MonitorStore::write_bl_ss(bufferlist& bl, const char *a, const char *b, bool append)
 {
-  int err = write_bl_ss_impl(bl, a, b, append);
-  if (err)
-    derr << "write_bl_ss " << a << "/" << b << " got error " << cpp_strerror(err) << dendl;
-  assert(!err);  // for now
-  return 0;
+  write_bl_ss_impl(bl, a, b, append);
 }
 
-int MonitorStore::put_bl_sn_map(const char *a,
+void MonitorStore::put_bl_sn_map(const char *a,
                                map<version_t,bufferlist>::iterator start,
                                map<version_t,bufferlist>::iterator end,
                                map<version_t,version_t> *gvmap)
@@ -426,13 +418,11 @@ int MonitorStore::put_bl_sn_map(const char *a,
       last - first < (unsigned)g_conf->mon_sync_fs_threshold) {
     // just do them individually
     for (map<version_t,bufferlist>::iterator p = start; p != end; ++p) {
-      int err = put_bl_sn(p->second, a, p->first);
-      if (err < 0)
-       return err;
+      put_bl_sn(p->second, a, p->first);
       if (gvmap && gvmap->count(p->first) && (*gvmap)[p->first] > 0)
        put_global_version(a, p->first, (*gvmap)[p->first]);
     }
-    return 0;
+    return;
   }
 
   // make sure dir exists
@@ -443,7 +433,7 @@ int MonitorStore::put_bl_sn_map(const char *a,
     err = -errno;
     derr << __func__ << " failed to create dir " << dfn << ": "
         << cpp_strerror(err) << dendl;
-    return err;
+    assert(0 == "failed to create dir");
   }
 
   for (map<version_t,bufferlist>::iterator p = start; p != end; ++p) {
@@ -456,14 +446,14 @@ int MonitorStore::put_bl_sn_map(const char *a,
     if (fd < 0) {
       int err = -errno;
       derr << "failed to open " << tfn << ": " << cpp_strerror(err) << dendl;
-      return err;
+      assert(0 == "failed to open");
     }
 
     err = p->second.write_fd(fd);
     close_err = TEMP_FAILURE_RETRY(::close(fd));
     assert (0 == close_err);
     if (err < 0)
-      return -errno;
+      assert(0 == "failed to write");
 
     // this doesn't try to be efficient.. too bad for you!  it may also
     // extend beyond commmitted, but that's okay; we only look at these
@@ -477,7 +467,7 @@ int MonitorStore::put_bl_sn_map(const char *a,
   if (dirfd < 0) {
     err = -errno;
     derr << "failed to open " << dir << ": " << cpp_strerror(err) << dendl;
-    return err;
+    assert(0 == "failed to open temp file");
   }
   sync_filesystem(dirfd);
   close_err = TEMP_FAILURE_RETRY(::close(dirfd));
@@ -492,7 +482,7 @@ int MonitorStore::put_bl_sn_map(const char *a,
     
     err = ::rename(tfn, fn);
     if (err < 0)
-      return -errno;
+      assert(0 == "failed to rename");
   }
     
   // fsync the dir (to commit the renames)
@@ -501,19 +491,17 @@ int MonitorStore::put_bl_sn_map(const char *a,
     err = -errno;
     derr << __func__ << " failed to open " << dir
         << ": " << cpp_strerror(err) << dendl;
-    return err;
+    assert(0 == "failed to open dir");
   }
   err = ::fsync(dirfd);
   if (err < 0) {
     err = -errno;
     derr << __func__ << " failed to fsync " << dir
         << ": " << cpp_strerror(err) << dendl;
-    return err;
+    assert(0 == "failed to fsync");
   }
   close_err = TEMP_FAILURE_RETRY(::close(dirfd));
   assert (0 == close_err);
-
-  return 0;
 }
 
 void MonitorStore::sync()
@@ -523,7 +511,7 @@ void MonitorStore::sync()
     int err = -errno;
     derr << __func__ << " failed to open " << dir
         << ": " << cpp_strerror(err) << dendl;
-    return;
+    assert(0 == "failed to open dir for syncing");
   }
   sync_filesystem(dirfd);
   int close_err = TEMP_FAILURE_RETRY(::close(dirfd));

diff --git a/src/mon/MonitorStore.h b/src/mon/MonitorStore.h
index 2f2426e587bfb2d00e124ae137ed67c4512abbf8..a77bba303d166f54c0e6f67ce22ab9bd864f1f20 100644 (file)
--- a/src/mon/MonitorStore.h
+++ b/src/mon/MonitorStore.h
@@ -26,9 +26,9 @@ class MonitorStore {
   string dir;
   int lock_fd;
 
-  int write_bl_ss_impl(bufferlist& bl, const char *a, const char *b,
+  void write_bl_ss_impl(bufferlist& bl, const char *a, const char *b,
                       bool append);
-  int write_bl_ss(bufferlist& bl, const char *a, const char *b,
+  void write_bl_ss(bufferlist& bl, const char *a, const char *b,
                  bool append);
 public:
   MonitorStore(const std::string &d) : dir(d), lock_fd(-1) { }
@@ -54,11 +54,11 @@ public:
     int ret = get_bl_ss(bl, a, b);
     assert (ret >= 0 || ret == -ENOENT);
   }
-  int put_bl_ss(bufferlist& bl, const char *a, const char *b) {
-    return write_bl_ss(bl, a, b, false);
+  void put_bl_ss(bufferlist& bl, const char *a, const char *b) {
+    write_bl_ss(bl, a, b, false);
   }
-  int append_bl_ss(bufferlist& bl, const char *a, const char *b) {
-    return write_bl_ss(bl, a, b, true);
+  void append_bl_ss(bufferlist& bl, const char *a, const char *b) {
+    write_bl_ss(bl, a, b, true);
   }
   bool exists_bl_sn(const char *a, version_t b) {
     char bs[20];
@@ -74,10 +74,10 @@ public:
     int ret = get_bl_sn(bl, a, b);
     assert(ret >= 0 || ret == -ENOENT);
   }
-  int put_bl_sn(bufferlist& bl, const char *a, version_t b) {
+  void put_bl_sn(bufferlist& bl, const char *a, version_t b) {
     char bs[20];
     snprintf(bs, sizeof(bs), "%llu", (unsigned long long)b);
-    return put_bl_ss(bl, a, bs);
+    put_bl_ss(bl, a, bs);
   }
   /**
    * Put a whole set of values efficiently and safely.
@@ -86,16 +86,16 @@ public:
    * @param vals - map of int name -> values
    * @return 0 for success or negative error code
    */
-  int put_bl_sn_map(const char *a,
+  void put_bl_sn_map(const char *a,
                    map<version_t,bufferlist>::iterator start,
                    map<version_t,bufferlist>::iterator end,
                    map<version_t,version_t> *gvmap);
 
-  int erase_ss(const char *a, const char *b);
-  int erase_sn(const char *a, version_t b) {
+  void erase_ss(const char *a, const char *b);
+  void erase_sn(const char *a, version_t b) {
     char bs[20];
     snprintf(bs, sizeof(bs), "%llu", (unsigned long long)b);
-    return erase_ss(a, bs);
+    erase_ss(a, bs);
   }
 
   /*