ceph-objectstore-tool: Clean up error handling

Use negative errors throughout and positive error to specify exit status
cpp_strerror() handles negative errors
Clean up main() code to use local "ret"
Let end of main() decide how to treat the final value of "ret"

Signed-off-by: David Zafman <dzafman@redhat.com>
(cherry picked from commit a50679a1a3313e4090bf24f6e4dda28e7d67d731)

Adjust back cherry-pick f237ed97228839a1b412ad213945f0343df05bf5
    Switched to use ret as in the original change from master

diff --git a/src/tools/ceph_objectstore_tool.cc b/src/tools/ceph_objectstore_tool.cc
index c8df7ac0ed7bb8306520248abe9391dc00a74bfd..6edec1c6c43416e99cd222da3d036237962168ee 100644 (file)
--- a/src/tools/ceph_objectstore_tool.cc
+++ b/src/tools/ceph_objectstore_tool.cc
@@ -608,8 +608,8 @@ static int get_fd_data(int fd, bufferlist &bl)
   do {
     ssize_t bytes = bl.read_fd(fd, max_read);
     if (bytes < 0) {
-      cerr << "read_fd error " << cpp_strerror(-bytes) << std::endl;
-      return 1;
+      cerr << "read_fd error " << cpp_strerror(bytes) << std::endl;
+      return bytes;
     }
 
     if (bytes == 0)
@@ -652,7 +652,7 @@ int get_log(ObjectStore *fs, __u8 struct_ver,
   }
   catch (const buffer::error &e) {
     cerr << "read_log threw exception error " << e.what() << std::endl;
-    return 1;
+    return -EFAULT;
   }
   return 0;
 }
@@ -711,7 +711,7 @@ int finish_remove_pgs(ObjectStore *store)
   vector<coll_t> ls;
   int r = store->list_collections(ls);
   if (r < 0) {
-    cerr << "finish_remove_pgs: failed to list pgs: " << cpp_strerror(-r)
+    cerr << "finish_remove_pgs: failed to list pgs: " << cpp_strerror(r)
       << std::endl;
     return r;
   }
@@ -762,7 +762,7 @@ int mark_pg_for_removal(ObjectStore *fs, spg_t pgid, ObjectStore::Transaction *t
   __u8 struct_v;
   r = PG::read_info(fs, pgid, coll, bl, info, past_intervals, struct_v);
   if (r < 0) {
-    cerr << __func__ << " error on read_info " << cpp_strerror(-r) << std::endl;
+    cerr << __func__ << " error on read_info " << cpp_strerror(r) << std::endl;
     return r;
   }
   if (struct_v < 8) {
@@ -813,7 +813,7 @@ int header::get_header()
   bytes = ebl.read_fd(file_fd, sh.header_size);
   if ((size_t)bytes != sh.header_size) {
     cerr << "Unexpected EOF" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   decode(ebliter);
@@ -830,14 +830,14 @@ int footer::get_footer()
   bytes = ebl.read_fd(file_fd, sh.footer_size);
   if ((size_t)bytes != sh.footer_size) {
     cerr << "Unexpected EOF" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   decode(ebliter);
 
   if (magic != endmagic) {
     cerr << "Bad footer magic" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   return 0;
@@ -854,7 +854,6 @@ int write_info(ObjectStore::Transaction &t, epoch_t epoch, pg_info_t &info,
     past_intervals,
     pgmeta_oid,
     true);
-  if (ret < 0) ret = -ret;
   if (ret) cerr << "Failed to write info" << std::endl;
   return ret;
 }
@@ -960,7 +959,7 @@ int export_file(ObjectStore *store, coll_t cid, ghobject_t &obj)
   bufferlist hdrbuf;
   ret = store->omap_get_header(cid, obj, &hdrbuf, true);
   if (ret < 0) {
-    cerr << "omap_get_header: " << cpp_strerror(-ret) << std::endl;
+    cerr << "omap_get_header: " << cpp_strerror(ret) << std::endl;
     return ret;
   }
 
@@ -972,7 +971,7 @@ int export_file(ObjectStore *store, coll_t cid, ghobject_t &obj)
   ObjectMap::ObjectMapIterator iter = store->get_omap_iterator(cid, obj);
   if (!iter) {
     ret = -ENOENT;
-    cerr << "omap_get_iterator: " << cpp_strerror(-ret) << std::endl;
+    cerr << "omap_get_iterator: " << cpp_strerror(ret) << std::endl;
     return ret;
   }
   iter->seek_to_first();
@@ -1029,7 +1028,7 @@ int get_osdmap(ObjectStore *store, epoch_t e, OSDMap &osdmap, bufferlist& bl)
       META_COLL, OSD::get_osdmap_pobject_name(e), 0, 0, bl) >= 0;
   if (!found) {
     cerr << "Can't find OSDMap for pg epoch " << e << std::endl;
-    return ENOENT;
+    return -ENOENT;
   }
   osdmap.decode(bl);
   if (debug)
@@ -1123,7 +1122,7 @@ int super_header::read_super()
   bytes = ebl.read_fd(file_fd, super_header::FIXED_LENGTH);
   if ((size_t)bytes != super_header::FIXED_LENGTH) {
     cerr << "Unexpected EOF" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   decode(ebliter);
@@ -1146,7 +1145,7 @@ int read_section(int fd, sectiontype_t *type, bufferlist *bl)
   bytes = bl->read_fd(fd, hdr.size);
   if (bytes != hdr.size) {
     cerr << "Unexpected EOF" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   if (hdr.size > 0) {
@@ -1261,7 +1260,7 @@ int skip_object(bufferlist &bl)
       done = true;
       break;
     default:
-      return EFAULT;
+      return -EFAULT;
     }
   }
   return 0;
@@ -1354,7 +1353,7 @@ int get_object_rados(librados::IoCtx &ioctx, bufferlist &bl)
       if (need_align) {
         if (ds.offset != in_offset) {
           cerr << "Discontiguous object data in export" << std::endl;
-          return EFAULT;
+          return -EFAULT;
         }
         assert(ds.databl.length() == ds.len);
         databl.claim_append(ds.databl);
@@ -1439,7 +1438,7 @@ int get_object_rados(librados::IoCtx &ioctx, bufferlist &bl)
       done = true;
       break;
     default:
-      return EFAULT;
+      return -EFAULT;
     }
   }
   return 0;
@@ -1471,12 +1470,12 @@ int get_object(ObjectStore *store, coll_t coll, bufferlist &bl, OSDMap &curmap)
     snapid_t coll_snap;
     if (coll.is_pg(coll_pgid, coll_snap) == false) {
       cerr << "INTERNAL ERROR: Bad collection during import" << std::endl;
-      return 1;
+      return -EFAULT;
     }
     if (coll_pgid.shard != ob.hoid.shard_id) {
       cerr << "INTERNAL ERROR: Importing shard " << coll_pgid.shard 
         << " but object shard is " << ob.hoid.shard_id << std::endl;
-      return 1;
+      return -EFAULT;
     }
      
     if (coll_pgid.pgid != pgid) {
@@ -1525,7 +1524,7 @@ int get_object(ObjectStore *store, coll_t coll, bufferlist &bl, OSDMap &curmap)
       done = true;
       break;
     default:
-      return EFAULT;
+      return -EFAULT;
     }
   }
   store->apply_transaction(*t);
@@ -1583,7 +1582,7 @@ int get_pg_metadata(ObjectStore *store, bufferlist &bl, metadata_section &ms,
 
   if (ms.map_epoch > sb.current_epoch) {
     cerr << "ERROR: Export map_epoch " << ms.map_epoch << " > osd epoch " << sb.current_epoch << std::endl;
-    return 1;
+    return -EINVAL;
   }
 
   // If the osdmap was present in the metadata we can check for splits.
@@ -1636,12 +1635,12 @@ int do_import_rados(string pool)
 
   if (sh.magic != super_header::super_magic) {
     cerr << "Invalid magic number" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   if (sh.version > super_header::super_ver) {
     cerr << "Can't handle export format version=" << sh.version << std::endl;
-    return EINVAL;
+    return -EINVAL;
   }
 
   //First section must be TYPE_PG_BEGIN
@@ -1650,7 +1649,7 @@ int do_import_rados(string pool)
   if (ret)
     return ret;
   if (type != TYPE_PG_BEGIN) {
-    return EFAULT;
+    return -EFAULT;
   }
 
   bufferlist::iterator ebliter = ebl.begin();
@@ -1672,7 +1671,7 @@ int do_import_rados(string pool)
   if (sb.compat_features.compare(pgb.superblock.compat_features) == -1) {
     cerr << "Export has incompatible features set "
       << pgb.superblock.compat_features << std::endl;
-    return 1;
+    return -EINVAL;
   }
 #endif
 
@@ -1733,7 +1732,7 @@ int do_import_rados(string pool)
       done = true;
       break;
     default:
-      return EFAULT;
+      return -EFAULT;
     }
   }
 
@@ -1790,12 +1789,12 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
 
   if (sh.magic != super_header::super_magic) {
     cerr << "Invalid magic number" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   if (sh.version > super_header::super_ver) {
     cerr << "Can't handle export format version=" << sh.version << std::endl;
-    return EINVAL;
+    return -EINVAL;
   }
 
   //First section must be TYPE_PG_BEGIN
@@ -1804,7 +1803,7 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
   if (ret)
     return ret;
   if (type != TYPE_PG_BEGIN) {
-    return EFAULT;
+    return -EFAULT;
   }
 
   bufferlist::iterator ebliter = ebl.begin();
@@ -1816,7 +1815,7 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
       && pgb.superblock.cluster_fsid != sb.cluster_fsid) {
     cerr << "Export came from different cluster with fsid "
          << pgb.superblock.cluster_fsid << std::endl;
-    return 1;
+    return -EINVAL;
   }
 
   if (debug) {
@@ -1838,11 +1837,11 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
       cerr << "OSD requires sharding to be enabled" << std::endl;
       cerr << std::endl;
       cerr << "If you wish to import, first do 'ceph-objectstore-tool...--op set-allow-sharded-objects'" << std::endl;
-      return 1;
+      return -EINVAL;
     }
     // Let them import if they specify the --force option
     if (!force)
-        return 11;  // Assume no +EAGAIN gets to end of main() until we clean up error code handling
+        return 11;  // Positive return means exit status
   }
 
   // Don't import if pool no longer exists
@@ -1856,7 +1855,7 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
   if (!curmap.have_pg_pool(pgid.pgid.m_pool)) {
     cerr << "Pool " << pgid.pgid.m_pool << " no longer exists" << std::endl;
     // Special exit code for this error, used by test code
-    return 10;  // Assume no +ECHILD gets to end of main() until we clean up error code handling
+    return 10;  // Positive return means exit status
   }
 
   ghobject_t pgmeta_oid = pgid.make_pgmeta_oid();
@@ -1867,7 +1866,7 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
   coll_t coll(pgid);
   if (store->collection_exists(coll)) {
     cerr << "pgid " << pgid << " already exists" << std::endl;
-    return 1;
+    return -EEXIST;
   }
 
   ObjectStore::Transaction *t = new ObjectStore::Transaction;
@@ -1911,13 +1910,13 @@ int do_import(ObjectStore *store, OSDSuperblock& sb, bool force)
       done = true;
       break;
     default:
-      return EFAULT;
+      return -EFAULT;
     }
   }
 
   if (!found_metadata) {
     cerr << "Missing metadata section" << std::endl;
-    return EFAULT;
+    return -EFAULT;
   }
 
   pg_log_t newlog, reject;
@@ -1993,7 +1992,7 @@ int do_remove_object(ObjectStore *store, coll_t coll, ghobject_t &ghobj)
 
   int r = store->stat(coll, ghobj, &st);
   if (r < 0) {
-    cerr << "remove: " << cpp_strerror(-r) << std::endl;
+    cerr << "remove: " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2001,8 +2000,8 @@ int do_remove_object(ObjectStore *store, coll_t coll, ghobject_t &ghobj)
   OSDriver::OSTransaction _t(driver.get_transaction(t));
   cout << "remove " << ghobj << std::endl;
   r = mapper.remove_oid(ghobj.hobj, &_t);
-  if (r != 0 && r != -ENOENT) {
-    cerr << "remove_oid returned " << cpp_strerror(-r) << std::endl;
+  if (r < 0 && r != -ENOENT) {
+    cerr << "remove_oid returned " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2018,7 +2017,7 @@ int do_list_attrs(ObjectStore *store, coll_t coll, ghobject_t &ghobj)
   map<string,bufferptr> aset;
   int r = store->getattrs(coll, ghobj, aset);
   if (r < 0) {
-    cerr << "getattrs: " << cpp_strerror(-r) << std::endl;
+    cerr << "getattrs: " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2060,8 +2059,8 @@ int do_get_bytes(ObjectStore *store, coll_t coll, ghobject_t &ghobj, int fd)
 
   int ret = store->stat(coll, ghobj, &st);
   if (ret < 0) {
-    cerr << "get-bytes: " << cpp_strerror(-ret) << std::endl;
-    return 1;
+    cerr << "get-bytes: " << cpp_strerror(ret) << std::endl;
+    return ret;
   }
 
   total = st.st_size;
@@ -2091,7 +2090,7 @@ int do_get_bytes(ObjectStore *store, coll_t coll, ghobject_t &ghobj, int fd)
     ret = write(fd, rawdatabl.c_str(), ret);
     if (ret == -1) {
       perror("write");
-      return 1;
+      return -errno;
     }
   }
 
@@ -2115,8 +2114,8 @@ int do_set_bytes(ObjectStore *store, coll_t coll, ghobject_t &ghobj, int fd)
     rawdatabl.clear();
     ssize_t bytes = rawdatabl.read_fd(fd, max_read);
     if (bytes < 0) {
-      cerr << "read_fd error " << cpp_strerror(-bytes) << std::endl;
-      return 1;
+      cerr << "read_fd error " << cpp_strerror(bytes) << std::endl;
+      return bytes;
     }
 
     if (bytes == 0)
@@ -2140,7 +2139,7 @@ int do_get_attr(ObjectStore *store, coll_t coll, ghobject_t &ghobj, string key)
 
   int r = store->getattr(coll, ghobj, key.c_str(), bp);
   if (r < 0) {
-    cerr << "getattr: " << cpp_strerror(-r) << std::endl;
+    cerr << "getattr: " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2163,8 +2162,9 @@ int do_set_attr(ObjectStore *store, coll_t coll, ghobject_t &ghobj, string key,
   if (debug)
     cerr << "Setattr " << ghobj << std::endl;
 
-  if (get_fd_data(fd, bl))
-    return 1;
+  int ret = get_fd_data(fd, bl);
+  if (ret < 0)
+    return ret;
 
   t->touch(coll, ghobj);
 
@@ -2197,7 +2197,7 @@ int do_get_omap(ObjectStore *store, coll_t coll, ghobject_t &ghobj, string key)
 
   int r = store->omap_get_values(coll, ghobj, keys, &out);
   if (r < 0) {
-    cerr << "omap_get_values: " << cpp_strerror(-r) << std::endl;
+    cerr << "omap_get_values: " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2229,8 +2229,9 @@ int do_set_omap(ObjectStore *store, coll_t coll, ghobject_t &ghobj, string key,
   if (debug)
     cerr << "Set_omap " << ghobj << std::endl;
 
-  if (get_fd_data(fd, valbl))
-    return 1;
+  int ret = get_fd_data(fd, valbl);
+  if (ret < 0)
+    return ret;
 
   attrset.insert(pair<string, bufferlist>(key, valbl));
 
@@ -2265,7 +2266,7 @@ int do_get_omaphdr(ObjectStore *store, coll_t coll, ghobject_t &ghobj)
 
   int r = store->omap_get_header(coll, ghobj, &hdrbl, true);
   if (r < 0) {
-    cerr << "omap_get_header: " << cpp_strerror(-r) << std::endl;
+    cerr << "omap_get_header: " << cpp_strerror(r) << std::endl;
     return r;
   }
 
@@ -2288,8 +2289,9 @@ int do_set_omaphdr(ObjectStore *store, coll_t coll, ghobject_t &ghobj, int fd)
   if (debug)
     cerr << "Omap_setheader " << ghobj << std::endl;
 
-  if (get_fd_data(fd, hdrbl))
-    return 1;
+  int ret = get_fd_data(fd, hdrbl);
+  if (ret)
+    return ret;
 
   t->touch(coll, ghobj);
 
@@ -2432,7 +2434,7 @@ int main(int argc, char **argv)
                                                   po::include_positional);
   } catch(po::error &e) {
     std::cerr << e.what() << std::endl;
-    return 1;
+    myexit(1);
   }
 
   if (vm.count("help")) {
@@ -2487,7 +2489,7 @@ int main(int argc, char **argv)
       file_fd = open(arg1.c_str(), O_RDONLY);
       if (file_fd < 0) {
         perror("open");
-        return 1;
+       myexit(1);
       }
     }
 
@@ -2556,12 +2558,12 @@ int main(int argc, char **argv)
 
   if (vm.count("file") && file_fd == fd_none) {
     cerr << "--file option only applies to import or export" << std::endl;
-    return 1;
+    myexit(1);
   }
 
   if (file_fd != fd_none && file_fd < 0) {
     perror("open");
-    return 1;
+    myexit(1);
   }
 
   osflagbits_t flags = 0;
@@ -2653,19 +2655,18 @@ int main(int argc, char **argv)
     myexit(1);
   }
 
-  int r = fs->mount();
-  if (r < 0) {
-    if (r == -EBUSY) {
+  int ret = fs->mount();
+  if (ret < 0) {
+    if (ret == -EBUSY) {
       cerr << "OSD has the store locked" << std::endl;
     } else {
-      cerr << "Mount failed with '" << cpp_strerror(-r) << "'" << std::endl;
+      cerr << "Mount failed with '" << cpp_strerror(ret) << "'" << std::endl;
     }
     myexit(1);
   }
 
   bool fs_sharded_objects = fs->get_allow_sharded_objects();
 
-  int ret = 0;
   vector<coll_t> ls;
   vector<coll_t>::iterator it;
   CompatSet supported;
@@ -2679,9 +2680,9 @@ int main(int argc, char **argv)
   bufferlist bl;
   OSDSuperblock superblock;
   bufferlist::iterator p;
-  r = fs->read(META_COLL, OSD_SUPERBLOCK_POBJECT, 0, 0, bl);
-  if (r < 0) {
-    cerr << "Failure to read OSD superblock error= " << r << std::endl;
+  ret = fs->read(META_COLL, OSD_SUPERBLOCK_POBJECT, 0, 0, bl);
+  if (ret < 0) {
+    cerr << "Failure to read OSD superblock: " << cpp_strerror(ret) << std::endl;
     goto out;
   }
 
@@ -2707,7 +2708,7 @@ int main(int argc, char **argv)
     CompatSet unsupported = supported.unsupported(superblock.compat_features);
     cerr << "On-disk OSD incompatible features set "
       << unsupported << std::endl;
-    ret = EINVAL;
+    ret = -EINVAL;
     goto out;
   }
 
@@ -2854,10 +2855,9 @@ int main(int argc, char **argv)
     bl.clear();
     ::encode(superblock, bl);
     t.write(META_COLL, OSD_SUPERBLOCK_POBJECT, 0, bl.length(), bl);
-    r = fs->apply_transaction(t);
-    if (r < 0) {
-      cerr << "Error writing OSD superblock: " << cpp_strerror(r) << std::endl;
-      ret = 1;
+    ret = fs->apply_transaction(t);
+    if (ret < 0) {
+      cerr << "Error writing OSD superblock: " << cpp_strerror(ret) << std::endl;
       goto out;
     }
 
@@ -2881,7 +2881,7 @@ int main(int argc, char **argv)
       cerr << "Found incomplete transition to sharded objects" << std::endl;
     cerr << std::endl;
     cerr << "Use --op set-allow-sharded-objects to repair" << std::endl;
-    ret = EINVAL;
+    ret = -EINVAL;
     goto out;
   }
 
@@ -2892,7 +2892,7 @@ int main(int argc, char **argv)
     }
     catch (const buffer::error &e) {
       cerr << "do_import threw exception error " << e.what() << std::endl;
-      ret = EFAULT;
+      ret = -EFAULT;
     }
     if (ret == EFAULT) {
       cerr << "Corrupt input for import" << std::endl;
@@ -2904,12 +2904,12 @@ int main(int argc, char **argv)
     // Undocumented feature to dump journal with mounted fs
     // This doesn't support the format option, but it uses the
     // ObjectStore::dump_journal() and mounts to get replay to run.
-    int r = fs->dump_journal(cout);
-    if (r) {
-      if (r == -EOPNOTSUPP) {
+    ret = fs->dump_journal(cout);
+    if (ret) {
+      if (ret == -EOPNOTSUPP) {
         cerr << "Object store type \"" << type << "\" doesn't support journal dump" << std::endl;
       } else {
-        cerr << "Journal dump failed with error " << r << std::endl;
+        cerr << "Journal dump failed with error " << cpp_strerror(ret) << std::endl;
       }
     }
     goto out;
@@ -2920,10 +2920,9 @@ int main(int argc, char **argv)
 
   if (op == "remove") {
     finish_remove_pgs(fs);
-    int r = initiate_new_remove_pg(fs, pgid);
-    if (r) {
+    ret = initiate_new_remove_pg(fs, pgid);
+    if (ret < 0) {
       cerr << "PG '" << pgid << "' not found" << std::endl;
-      ret = 1;
       goto out;
     }
     finish_remove_pgs(fs);
@@ -2945,18 +2944,16 @@ int main(int argc, char **argv)
   }
 
   if (op == "list") {
-    r = do_list(fs, pgidstr, object, formatter, debug, human_readable);
-    if (r) {
-      cerr << "do_list failed with " << r << std::endl;
-      ret = 1;
+    ret = do_list(fs, pgidstr, object, formatter, debug, human_readable);
+    if (ret < 0) {
+      cerr << "do_list failed: " << cpp_strerror(ret) << std::endl;
     }
     goto out;
   }
 
-  r = fs->list_collections(ls);
-  if (r < 0) {
-    cerr << "failed to list pgs: " << cpp_strerror(-r) << std::endl;
-    ret = 1;
+  ret = fs->list_collections(ls);
+  if (ret < 0) {
+    cerr << "failed to list pgs: " << cpp_strerror(ret) << std::endl;
     goto out;
   }
 
@@ -3008,25 +3005,15 @@ int main(int argc, char **argv)
     if (vm.count("objcmd")) {
       ret = 0;
       if (objcmd == "remove") {
-        int r = do_remove_object(fs, coll, ghobj);
-        if (r) {
-          ret = 1;
-        }
+        ret = do_remove_object(fs, coll, ghobj);
         goto out;
       } else if (objcmd == "list-attrs") {
-        int r = do_list_attrs(fs, coll, ghobj);
-        if (r) {
-          ret = 1;
-        }
+        ret = do_list_attrs(fs, coll, ghobj);
         goto out;
       } else if (objcmd == "list-omap") {
-        int r = do_list_omap(fs, coll, ghobj);
-        if (r) {
-          ret = 1;
-        }
+        ret = do_list_omap(fs, coll, ghobj);
         goto out;
       } else if (objcmd == "get-bytes" || objcmd == "set-bytes") {
-        int r;
         if (objcmd == "get-bytes") {
           int fd;
           if (vm.count("arg1") == 0 || arg1 == "-") {
@@ -3039,7 +3026,7 @@ int main(int argc, char **argv)
               goto out;
             }
           }
-          r = do_get_bytes(fs, coll, ghobj, fd);
+          ret = do_get_bytes(fs, coll, ghobj, fd);
           if (fd != STDOUT_FILENO)
             close(fd);
         } else {
@@ -3060,12 +3047,10 @@ int main(int argc, char **argv)
               goto out;
             }
           }
-          r = do_set_bytes(fs, coll, ghobj, fd);
+          ret = do_set_bytes(fs, coll, ghobj, fd);
           if (fd != STDIN_FILENO)
             close(fd);
         }
-        if (r)
-          ret = 1;
         goto out;
       } else if (objcmd == "get-attr") {
        if (vm.count("arg1") == 0) {
@@ -3073,9 +3058,7 @@ int main(int argc, char **argv)
           ret = 1;
           goto out;
         }
-       r = do_get_attr(fs, coll, ghobj, arg1);
-       if (r)
-         ret = 1;
+       ret = do_get_attr(fs, coll, ghobj, arg1);
         goto out;
       } else if (objcmd == "set-attr") {
        if (vm.count("arg1") == 0) {
@@ -3100,11 +3083,9 @@ int main(int argc, char **argv)
            goto out;
          }
        }
-       r = do_set_attr(fs, coll, ghobj, arg1, fd);
+       ret = do_set_attr(fs, coll, ghobj, arg1, fd);
        if (fd != STDIN_FILENO)
          close(fd);
-       if (r)
-         ret = 1;
         goto out;
       } else if (objcmd == "rm-attr") {
        if (vm.count("arg1") == 0) {
@@ -3112,9 +3093,7 @@ int main(int argc, char **argv)
           ret = 1;
           goto out;
         }
-       r = do_rm_attr(fs, coll, ghobj, arg1);
-       if (r)
-         ret = 1;
+       ret = do_rm_attr(fs, coll, ghobj, arg1);
         goto out;
       } else if (objcmd == "get-omap") {
        if (vm.count("arg1") == 0) {
@@ -3122,9 +3101,7 @@ int main(int argc, char **argv)
           ret = 1;
           goto out;
         }
-       r = do_get_omap(fs, coll, ghobj, arg1);
-       if (r)
-         ret = 1;
+       ret = do_get_omap(fs, coll, ghobj, arg1);
         goto out;
       } else if (objcmd == "set-omap") {
        if (vm.count("arg1") == 0) {
@@ -3149,11 +3126,9 @@ int main(int argc, char **argv)
            goto out;
          }
        }
-       r = do_set_omap(fs, coll, ghobj, arg1, fd);
+       ret = do_set_omap(fs, coll, ghobj, arg1, fd);
        if (fd != STDIN_FILENO)
          close(fd);
-       if (r)
-         ret = 1;
         goto out;
       } else if (objcmd == "rm-omap") {
        if (vm.count("arg1") == 0) {
@@ -3161,9 +3136,7 @@ int main(int argc, char **argv)
           ret = 1;
           goto out;
         }
-       r = do_rm_omap(fs, coll, ghobj, arg1);
-       if (r)
-         ret = 1;
+       ret = do_rm_omap(fs, coll, ghobj, arg1);
         goto out;
       } else if (objcmd == "get-omaphdr") {
        if (vm.count("arg1")) {
@@ -3171,9 +3144,7 @@ int main(int argc, char **argv)
           ret = 1;
           goto out;
         }
-       r = do_get_omaphdr(fs, coll, ghobj);
-       if (r)
-         ret = 1;
+       ret = do_get_omaphdr(fs, coll, ghobj);
         goto out;
       } else if (objcmd == "set-omaphdr") {
         // Extra arg
@@ -3199,11 +3170,9 @@ int main(int argc, char **argv)
            goto out;
          }
        }
-       r = do_set_omaphdr(fs, coll, ghobj, fd);
+       ret = do_set_omaphdr(fs, coll, ghobj, fd);
        if (fd != STDIN_FILENO)
          close(fd);
-       if (r)
-         ret = 1;
         goto out;
       }
       cerr << "Unknown object command '" << objcmd << "'" << std::endl;
@@ -3214,8 +3183,8 @@ int main(int argc, char **argv)
 
     bufferlist bl;
     map_epoch = 0;
-    r = PG::peek_map_epoch(fs, pgid, &map_epoch, &bl);
-    if (r < 0)
+    ret = PG::peek_map_epoch(fs, pgid, &map_epoch, &bl);
+    if (ret < 0)
       cerr << "peek_map_epoch returns an error" << std::endl;
 
     if (debug)
@@ -3224,16 +3193,15 @@ int main(int argc, char **argv)
     pg_info_t info(pgid);
     map<epoch_t,pg_interval_t> past_intervals;
     __u8 struct_ver;
-    r = PG::read_info(fs, pgid, coll, bl, info, past_intervals,
+    ret = PG::read_info(fs, pgid, coll, bl, info, past_intervals,
                      struct_ver);
-    if (r < 0) {
-      cerr << "read_info error " << cpp_strerror(-r) << std::endl;
-      ret = 1;
+    if (ret < 0) {
+      cerr << "read_info error " << cpp_strerror(ret) << std::endl;
       goto out;
     }
     if (struct_ver < PG::compat_struct_v) {
       cerr << "PG is too old to upgrade, use older Ceph version" << std::endl;
-      ret = 1;
+      ret = -EFAULT;
       goto out;
     }
     if (debug)
@@ -3255,7 +3223,7 @@ int main(int argc, char **argv)
       map<eversion_t, hobject_t> divergent_priors;
       ret = get_log(fs, struct_ver, coll, pgid, info, log, missing,
                     divergent_priors);
-      if (ret > 0)
+      if (ret < 0)
           goto out;
 
       formatter->open_object_section("op_log");
@@ -3288,7 +3256,7 @@ int main(int argc, char **argv)
         cerr << "Can't remove past-intervals, version mismatch " << (int)struct_ver
           << " (pg)  != " << (int)PG::cur_struct_v << " (tool)"
           << std::endl;
-        ret = 1;
+        ret = -EFAULT;
         goto out;
       }
 
@@ -3309,16 +3277,18 @@ int main(int argc, char **argv)
     }
   } else {
     cerr << "PG '" << pgid << "' not found" << std::endl;
-    ret = 1;
+    ret = -ENOENT;
   }
 
 out:
-  if (fs->umount() < 0) {
-    cerr << "umount failed" << std::endl;
-    if (ret == 0) ret = 1;
+  int r = fs->umount();
+  if (r < 0) {
+    cerr << "umount failed: " << cpp_strerror(r) << std::endl;
+    // If no previous error, then use umount() error
+    if (ret == 0)
+      ret = r;
   }
 
-  // Check for -errno accidentally getting here
   if (ret < 0)
     ret = 1;
   myexit(ret);