filestore: audit + clean up error checks

- use temp var for errno
- in general return -errno from helpers

Signed-off-by: Sage Weil <sage.weil@dreamhost.com>

diff --git a/src/os/FileStore.cc b/src/os/FileStore.cc
index c1ab28552a6310008938563300041dc0803978bc..175165c09f1f5ad99b0a48a9ac7c08485d54fd7b 100644 (file)
--- a/src/os/FileStore.cc
+++ b/src/os/FileStore.cc
@@ -1074,9 +1074,10 @@ int FileStore::lock_fsid()
   l.l_len = 0;
   int r = ::fcntl(fsid_fd, F_SETLK, &l);
   if (r < 0) {
-    char buf[80];
-    dout(0) << "lock_fsid failed to lock " << basedir << "/fsid, is another ceph-osd still running? " << strerror_r(errno, buf, sizeof(buf)) << dendl;
-    return -errno;
+    int err = errno;
+    dout(0) << "lock_fsid failed to lock " << basedir << "/fsid, is another ceph-osd still running? "
+           << cpp_strerror(err) << dendl;
+    return -err;
   }
   return 0;
 }
@@ -1156,7 +1157,6 @@ int FileStore::_detect_fs()
   blk_size = st.f_bsize;
 
 #if defined(__linux__)
-  char buf[80];
   static const __SWORD_TYPE BTRFS_F_TYPE(0x9123683E);
   if (st.f_type == BTRFS_F_TYPE) {
     dout(0) << "mount detected btrfs" << dendl;      
@@ -1172,7 +1172,7 @@ int FileStore::_detect_fs()
        dout(0) << "mount btrfs CLONE_RANGE ioctl is supported" << dendl;
       } else {
        btrfs_clone_range = false;
-       dout(0) << "mount btrfs CLONE_RANGE ioctl is NOT supported: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+       dout(0) << "mount btrfs CLONE_RANGE ioctl is NOT supported: " << cpp_strerror(r) << dendl;
       }
     } else {
       dout(0) << "mount btrfs CLONE_RANGE ioctl is DISABLED via 'filestore btrfs clone range' option" << dendl;
@@ -1183,6 +1183,7 @@ int FileStore::_detect_fs()
     volargs.fd = fd;
     strcpy(volargs.name, "sync_snap_test");
     r = ::ioctl(fd, BTRFS_IOC_SNAP_CREATE, &volargs);
+    int err = errno;
     if (r == 0 || errno == EEXIST) {
       dout(0) << "mount btrfs SNAP_CREATE is supported" << dendl;
       btrfs_snap_create = true;
@@ -1192,10 +1193,10 @@ int FileStore::_detect_fs()
        dout(0) << "mount btrfs SNAP_DESTROY is supported" << dendl;
        btrfs_snap_destroy = true;
       } else {
-       dout(0) << "mount btrfs SNAP_DESTROY failed: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+       dout(0) << "mount btrfs SNAP_DESTROY failed: " << cpp_strerror(err) << dendl;
       }
     } else {
-      dout(0) << "mount btrfs SNAP_CREATE failed: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+      dout(0) << "mount btrfs SNAP_CREATE failed: " << cpp_strerror(err) << dendl;
     }
 
     if (m_filestore_btrfs_snap && !btrfs_snap_destroy) {
@@ -1211,20 +1212,25 @@ int FileStore::_detect_fs()
     // start_sync?
     __u64 transid = 0;
     r = ::ioctl(fd, BTRFS_IOC_START_SYNC, &transid);
-    dout(0) << "mount btrfs START_SYNC got " << r << " " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+    if (r < 0) {
+      int err = errno;
+      dout(0) << "mount btrfs START_SYNC got " << cpp_strerror(err) << dendl;
+    }
     if (r == 0 && transid > 0) {
       dout(0) << "mount btrfs START_SYNC is supported (transid " << transid << ")" << dendl;
 
       // do we have wait_sync too?
       r = ::ioctl(fd, BTRFS_IOC_WAIT_SYNC, &transid);
-      if (r == 0 || r == -ERANGE) {
+      if (r == 0 || errno == ERANGE) {
        dout(0) << "mount btrfs WAIT_SYNC is supported" << dendl;
        btrfs_wait_sync = true;
       } else {
-       dout(0) << "mount btrfs WAIT_SYNC is NOT supported: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+       int err = errno;
+       dout(0) << "mount btrfs WAIT_SYNC is NOT supported: " << cpp_strerror(err) << dendl;
       }
     } else {
-      dout(0) << "mount btrfs START_SYNC is NOT supported: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+      int err = errno;
+      dout(0) << "mount btrfs START_SYNC is NOT supported: " << cpp_strerror(err) << dendl;
     }
 
     if (btrfs_wait_sync) {
@@ -1243,12 +1249,13 @@ int FileStore::_detect_fs()
       if (::fstatat(fd, vol_args.name, &st, 0) == 0) {
        dout(0) << "mount btrfs removing old async_snap_test" << dendl;
        r = ::ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &vol_args);
-       if (r != 0)
-         dout(0) << "mount  failed to remove old async_snap_test: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+       if (r != 0) {
+         int err = errno;
+         dout(0) << "mount  failed to remove old async_snap_test: " << cpp_strerror(err) << dendl;
+       }
       }
 
       r = ::ioctl(fd, BTRFS_IOC_SNAP_CREATE_V2, &async_args);
-      dout(0) << "mount btrfs SNAP_CREATE_V2 got " << r << " " << strerror_r(-r, buf, sizeof(buf)) << dendl;
       if (r == 0 || errno == EEXIST) {
        dout(0) << "mount btrfs SNAP_CREATE_V2 is supported" << dendl;
        btrfs_snap_create_v2 = true;
@@ -1256,11 +1263,13 @@ int FileStore::_detect_fs()
        // clean up
        r = ::ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &vol_args);
        if (r != 0) {
-         dout(0) << "mount btrfs SNAP_DESTROY failed: " << strerror_r(-r, buf, sizeof(buf)) << dendl;
+         int err = errno;
+         dout(0) << "mount btrfs SNAP_DESTROY failed: " << cpp_strerror(err) << dendl;
        }
       } else {
+       int err = errno;
        dout(0) << "mount btrfs SNAP_CREATE_V2 is NOT supported: "
-               << strerror_r(-r, buf, sizeof(buf)) << dendl;
+               << cpp_strerror(err) << dendl;
       }
     }
 
@@ -1397,8 +1406,7 @@ int FileStore::read_op_seq(const char *fn, uint64_t *seq)
   memset(s, 0, sizeof(s));
   int ret = safe_read(op_fd, s, sizeof(s) - 1);
   if (ret < 0) {
-    derr << "error reading " << current_op_seq_fn << ": "
-        << cpp_strerror(ret) << dendl;
+    derr << "error reading " << current_op_seq_fn << ": " << cpp_strerror(ret) << dendl;
     TEMP_FAILURE_RETRY(::close(op_fd));
     return ret;
   }
@@ -1409,9 +1417,10 @@ int FileStore::read_op_seq(const char *fn, uint64_t *seq)
 int FileStore::write_op_seq(int fd, uint64_t seq)
 {
   char s[30];
-  int ret;
   snprintf(s, sizeof(s), "%" PRId64 "\n", seq);
-  ret = TEMP_FAILURE_RETRY(::pwrite(fd, s, strlen(s), 0));
+  int ret = TEMP_FAILURE_RETRY(::pwrite(fd, s, strlen(s), 0));
+  if (ret < 0)
+    return -errno;
   return ret;
 }
 
@@ -1433,7 +1442,7 @@ int FileStore::mount()
   if (::access(basedir.c_str(), R_OK | W_OK)) {
     ret = -errno;
     derr << "FileStore::mount: unable to access basedir '" << basedir << "': "
-           << cpp_strerror(ret) << dendl;
+        << cpp_strerror(ret) << dendl;
     goto done;
   }
 
@@ -1600,13 +1609,10 @@ int FileStore::mount()
 
       // drop current?
       if (curr_seq > 0) {
-       ret = ::ioctl(basedir_fd,
-                     BTRFS_IOC_SNAP_DESTROY,
-                     &vol_args);
+       ret = ::ioctl(basedir_fd, BTRFS_IOC_SNAP_DESTROY, &vol_args);
        if (ret) {
-         ret = errno;
-         derr << "FileStore::mount: error removing old current subvol: "
-              << cpp_strerror(ret) << dendl;
+         ret = -errno;
+         derr << "FileStore::mount: error removing old current subvol: " << cpp_strerror(ret) << dendl;
          char s[PATH_MAX];
          snprintf(s, sizeof(s), "%s/current.remove.me.%d", basedir.c_str(), rand());
          if (::rename(current_fn.c_str(), s)) {
@@ -1622,14 +1628,12 @@ int FileStore::mount()
       vol_args.fd = ::open(s, O_RDONLY);
       if (vol_args.fd < 0) {
        ret = -errno;
-       derr << "FileStore::mount: error opening '" << s << "': "
-            << cpp_strerror(ret) << dendl;
+       derr << "FileStore::mount: error opening '" << s << "': " << cpp_strerror(ret) << dendl;
        goto close_basedir_fd;
       }
       if (::ioctl(basedir_fd, BTRFS_IOC_SNAP_CREATE, &vol_args)) {
        ret = -errno;
-       derr << "FileStore::mount: error ioctl(BTRFS_IOC_SNAP_CREATE) failed: "
-            << cpp_strerror(ret) << dendl;
+       derr << "FileStore::mount: error ioctl(BTRFS_IOC_SNAP_CREATE) failed: " << cpp_strerror(ret) << dendl;
        TEMP_FAILURE_RETRY(::close(vol_args.fd));
        goto close_basedir_fd;
       }
@@ -1640,8 +1644,8 @@ int FileStore::mount()
 
   current_fd = ::open(current_fn.c_str(), O_RDONLY);
   if (current_fd < 0) {
-    derr << "FileStore::mount: error opening: " << current_fn << ": "
-        << cpp_strerror(ret) << dendl;
+    ret = -errno;
+    derr << "FileStore::mount: error opening: " << current_fn << ": " << cpp_strerror(ret) << dendl;
     goto close_basedir_fd;
   }
 
@@ -1721,8 +1725,7 @@ int FileStore::mount()
 
   ret = journal_replay(initial_op_seq);
   if (ret < 0) {
-    derr << "mount failed to open journal " << journalpath << ": "
-        << cpp_strerror(ret) << dendl;
+    derr << "mount failed to open journal " << journalpath << ": " << cpp_strerror(ret) << dendl;
     if (ret == -ENOTTY) {
       derr << "maybe journal is not pointing to a block device and its size "
           << "wasn't configured?" << dendl;
@@ -2149,20 +2152,19 @@ int FileStore::_transaction_start(uint64_t bytes, uint64_t ops)
       !m_filestore_btrfs_trans)
     return 0;
 
-  char buf[80];
   int fd = ::open(basedir.c_str(), O_RDONLY);
   if (fd < 0) {
-    dout(0) << "transaction_start got " << strerror_r(errno, buf, sizeof(buf))
-           << " from btrfs open" << dendl;
-    assert(0);
+    int err = errno;
+    dout(0) << "transaction_start got " << cpp_strerror(err) << " from btrfs open" << dendl;
+    assert(0 == "couldn't open basedir");
   }
 
   int r = ::ioctl(fd, BTRFS_IOC_TRANS_START);
   if (r < 0) {
-    dout(0) << "transaction_start got " << strerror_r(errno, buf, sizeof(buf))
-           << " from btrfs ioctl" << dendl;    
+    int err = errno;
+    dout(0) << "transaction_start got " << cpp_strerror(err) << " from btrfs ioctl" << dendl;    
     ::close(fd);
-    return -errno;
+    return -err;
   }
   dout(10) << "transaction_start " << fd << dendl;
 
@@ -2493,8 +2495,7 @@ int FileStore::read(coll_t cid, const hobject_t& oid,
   int fd = lfn_open(cid, oid, O_RDONLY);
   if (fd < 0) {
     int err = errno;
-    dout(10) << "FileStore::read(" << cid << "/" << oid << "): open error "
-            << cpp_strerror(err) << dendl;
+    dout(10) << "FileStore::read(" << cid << "/" << oid << ") open error: " << cpp_strerror(err) << dendl;
     return -err;
   }
 
@@ -2508,8 +2509,7 @@ int FileStore::read(coll_t cid, const hobject_t& oid,
   bufferptr bptr(len);  // prealloc space for entire read
   got = safe_pread(fd, bptr.c_str(), len, offset);
   if (got < 0) {
-    dout(10) << "FileStore::read(" << cid << "/" << oid << "): pread error "
-            << cpp_strerror(got) << dendl;
+    dout(10) << "FileStore::read(" << cid << "/" << oid << ") pread error: " << cpp_strerror(got) << dendl;
     TEMP_FAILURE_RETRY(::close(fd));
     return got;
   }
@@ -2542,9 +2542,8 @@ int FileStore::fiemap(coll_t cid, const hobject_t& oid,
   int r;
   int fd = lfn_open(cid, oid, O_RDONLY);
   if (fd < 0) {
-    char buf[80];
-    dout(10) << "read couldn't open " << cid << "/" << oid << " errno " << errno << " " << strerror_r(errno, buf, sizeof(buf)) << dendl;
     r = -errno;
+    dout(10) << "read couldn't open " << cid << "/" << oid << ": " << cpp_strerror(r) << dendl;
   } else {
     uint64_t i;
 
@@ -2643,20 +2642,20 @@ int FileStore::_write(coll_t cid, const hobject_t& oid,
 
   int64_t actual;
 
-  char buf[80];
   int flags = O_WRONLY|O_CREAT;
   int fd = lfn_open(cid, oid, flags, 0644);
   if (fd < 0) {
-    dout(0) << "write couldn't open " << cid << "/" << oid << " flags " << flags << " errno " << errno << " " << strerror_r(errno, buf, sizeof(buf)) << dendl;
     r = -errno;
+    dout(0) << "write couldn't open " << cid << "/" << oid << " flags " << flags << ": "
+           << cpp_strerror(r) << dendl;
     goto out;
   }
     
   // seek
   actual = ::lseek64(fd, offset, SEEK_SET);
   if (actual < 0) {
-    dout(0) << "write lseek64 to " << offset << " failed: " << strerror_r(errno, buf, sizeof(buf)) << dendl;
     r = -errno;
+    dout(0) << "write lseek64 to " << offset << " failed: " << cpp_strerror(r) << dendl;
     goto out;
   }
   if (actual != (int64_t)offset) {
@@ -3034,9 +3033,9 @@ void FileStore::sync_entry()
       sync_epoch++;
 
       dout(15) << "sync_entry committing " << cp << " sync_epoch " << sync_epoch << dendl;
-      if (write_op_seq(op_fd, cp) < 0) {
-       derr << "Error: " << cpp_strerror(errno) 
-            << " during write_op_seq" << dendl;
+      int err = write_op_seq(op_fd, cp);
+      if (err < 0) {
+       derr << "Error during write_op_seq: " << cpp_strerror(err) << dendl;
        assert(0);
       }
 
@@ -3052,11 +3051,14 @@ void FileStore::sync_entry()
 
          dout(10) << "taking async snap '" << async_args.name << "'" << dendl;
          int r = ::ioctl(basedir_fd, BTRFS_IOC_SNAP_CREATE_V2, &async_args);
-         char buf[100];
-         dout(20) << "async snap create '" << async_args.name
-                  << "' transid " << async_args.transid
-                  << " got " << r << " " << strerror_r(r < 0 ? errno : 0, buf, sizeof(buf)) << dendl;
-         assert(r == 0);
+         if (r < 0) {
+           int err = errno;
+           derr << "async snap create '" << async_args.name << "' transid " << async_args.transid
+                << " got " << cpp_strerror(err) << dendl;
+           assert(0 == "async snap ioctl error");
+         }
+         dout(20) << "async snap create '" << async_args.name << "' transid " << async_args.transid << dendl;
+
          snaps.push_back(cp);
 
          commit_started();
@@ -3131,9 +3133,8 @@ void FileStore::sync_entry()
          dout(10) << "removing snap '" << vol_args.name << "'" << dendl;
          int r = ::ioctl(basedir_fd, BTRFS_IOC_SNAP_DESTROY, &vol_args);
          if (r) {
-           char buf[100];
-           dout(20) << "unable to destroy snap '" << vol_args.name << "' got " << r
-                    << " " << strerror_r(r < 0 ? errno : 0, buf, sizeof(buf)) << dendl;
+           int err = errno;
+           derr << "unable to destroy snap '" << vol_args.name << "' got " << cpp_strerror(err) << dendl;
          }
        }
       }
@@ -3282,8 +3283,10 @@ int FileStore::snapshot(const string& name)
   snprintf(vol_args.name, sizeof(vol_args.name), CLUSTER_SNAP_ITEM, name.c_str());
 
   int r = ::ioctl(basedir_fd, BTRFS_IOC_SNAP_CREATE, &vol_args);
-  if (r)
+  if (r) {
+    r = -errno;
     derr << "snapshot " << name << " failed: " << cpp_strerror(r) << dendl;
+  }
 
   return r;
 }
@@ -3699,7 +3702,7 @@ int FileStore::list_collections(vector<coll_t>& ls)
       snprintf(filename, sizeof(filename), "%s/%s", fn, de->d_name);
 
       r = ::stat(filename, &sb);
-      if (r == -1) {
+      if (r < 0) {
        r = -errno;
        derr << "stat on " << filename << ": " << cpp_strerror(-r) << dendl;
        break;