mds: pass LogEvent as unique_ptr

To avoid leaks.

Fixes: http://tracker.ceph.com/issues/38324
Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>

diff --git a/src/mds/LogEvent.cc b/src/mds/LogEvent.cc
index 9a6e26beee67310df72d8b0ece934fc029b67b07..bafe4b4fbd5ea0d527da5a83aa03d2bb82f26cf7 100644 (file)
--- a/src/mds/LogEvent.cc
+++ b/src/mds/LogEvent.cc
@@ -41,12 +41,11 @@
 #define dout_context g_ceph_context
 
 
-LogEvent *LogEvent::decode(bufferlist& bl)
+std::unique_ptr<LogEvent> LogEvent::decode_event(bufferlist::const_iterator p)
 {
   // parse type, length
-  auto p = bl.cbegin();
   EventType type;
-  LogEvent *event = NULL;
+  std::unique_ptr<LogEvent> event;
   using ceph::decode;
   decode(type, p);
 
@@ -54,7 +53,7 @@ LogEvent *LogEvent::decode(bufferlist& bl)
     try {
       DECODE_START(1, p);
       decode(type, p);
-      event = decode_event(bl, p, type);
+      event = decode_event(p, type);
       DECODE_FINISH(p);
     }
     catch (const buffer::error &e) {
@@ -62,7 +61,7 @@ LogEvent *LogEvent::decode(bufferlist& bl)
       return NULL;
     }
   } else { // we are using classic encoding
-    event = decode_event(bl, p, type);
+    event = decode_event(p, type);
   }
   return event;
 }
@@ -126,43 +125,73 @@ LogEvent::EventType LogEvent::str_to_type(std::string_view str)
 }
 
 
-LogEvent *LogEvent::decode_event(bufferlist& bl, bufferlist::const_iterator& p, LogEvent::EventType type)
+std::unique_ptr<LogEvent> LogEvent::decode_event(bufferlist::const_iterator& p, LogEvent::EventType type)
 {
-  int length = bl.length() - p.get_off();
+  const auto length = p.get_remaining();
   generic_dout(15) << "decode_log_event type " << type << ", size " << length << dendl;
   
   // create event
-  LogEvent *le;
+  std::unique_ptr<LogEvent> le;
   switch (type) {
-  case EVENT_SUBTREEMAP: le = new ESubtreeMap; break;
+  case EVENT_SUBTREEMAP:
+    le = std::make_unique<ESubtreeMap>();
+    break;
   case EVENT_SUBTREEMAP_TEST: 
-    le = new ESubtreeMap;
+    le = std::make_unique<ESubtreeMap>();
     le->set_type(type);
     break;
-  case EVENT_EXPORT: le = new EExport; break;
-  case EVENT_IMPORTSTART: le = new EImportStart; break;
-  case EVENT_IMPORTFINISH: le = new EImportFinish; break;
-  case EVENT_FRAGMENT: le = new EFragment; break;
-
-  case EVENT_RESETJOURNAL: le = new EResetJournal; break;
-
-  case EVENT_SESSION: le = new ESession; break;
-  case EVENT_SESSIONS_OLD: le = new ESessions; (static_cast<ESessions *>(le))->mark_old_encoding(); break;
-  case EVENT_SESSIONS: le = new ESessions; break;
-
-  case EVENT_UPDATE: le = new EUpdate; break;
-  case EVENT_SLAVEUPDATE: le = new ESlaveUpdate; break;
-  case EVENT_OPEN: le = new EOpen; break;
-  case EVENT_COMMITTED: le = new ECommitted; break;
-
-  case EVENT_TABLECLIENT: le = new ETableClient; break;
-  case EVENT_TABLESERVER: le = new ETableServer; break;
-
-  case EVENT_NOOP: le = new ENoOp; break;
-
+  case EVENT_EXPORT:
+    le = std::make_unique<EExport>();
+    break;
+  case EVENT_IMPORTSTART:
+    le = std::make_unique<EImportStart>();
+    break;
+  case EVENT_IMPORTFINISH:
+    le = std::make_unique<EImportFinish>();
+    break;
+  case EVENT_FRAGMENT:
+    le = std::make_unique<EFragment>();
+    break;
+  case EVENT_RESETJOURNAL:
+    le = std::make_unique<EResetJournal>();
+    break;
+  case EVENT_SESSION:
+    le = std::make_unique<ESession>();
+    break;
+  case EVENT_SESSIONS_OLD:
+    {
+      auto e = std::make_unique<ESessions>();
+      e->mark_old_encoding();
+      le = std::move(e);
+    }
+    break;
+  case EVENT_SESSIONS:
+    le = std::make_unique<ESessions>();
+    break;
+  case EVENT_UPDATE:
+    le = std::make_unique<EUpdate>();
+    break;
+  case EVENT_SLAVEUPDATE:
+    le = std::make_unique<ESlaveUpdate>();
+    break;
+  case EVENT_OPEN:
+    le = std::make_unique<EOpen>();
+    break;
+  case EVENT_COMMITTED:
+    le = std::make_unique<ECommitted>();
+    break;
+  case EVENT_TABLECLIENT:
+    le = std::make_unique<ETableClient>();
+    break;
+  case EVENT_TABLESERVER:
+    le = std::make_unique<ETableServer>();
+    break;
+  case EVENT_NOOP:
+    le = std::make_unique<ENoOp>();
+    break;
   default:
     generic_dout(0) << "uh oh, unknown log event type " << type << " length " << length << dendl;
-    return NULL;
+    return nullptr;
   }
 
   // decode
@@ -171,8 +200,7 @@ LogEvent *LogEvent::decode_event(bufferlist& bl, bufferlist::const_iterator& p,
   }
   catch (const buffer::error &e) {
     generic_dout(0) << "failed to decode LogEvent type " << type << dendl;
-    delete le;
-    return NULL;
+    return nullptr;
   }
 
   ceph_assert(p.end());

diff --git a/src/mds/LogEvent.h b/src/mds/LogEvent.h
index 11aa4a82eb7635aaedb7c1cbb97704f9164d1163..f9133b9aac47a8d1f21f0f7beedc2b334077531d 100644 (file)
--- a/src/mds/LogEvent.h
+++ b/src/mds/LogEvent.h
@@ -58,7 +58,7 @@ public:
 private:
   EventType _type;
   uint64_t _start_off;
-  static LogEvent *decode_event(bufferlist& bl, bufferlist::const_iterator& p, EventType type);
+  static std::unique_ptr<LogEvent> decode_event(bufferlist::const_iterator&, EventType);
 
 protected:
   utime_t stamp;
@@ -87,8 +87,8 @@ public:
 
   // encoding
   virtual void encode(bufferlist& bl, uint64_t features) const = 0;
-  virtual void decode(bufferlist::const_iterator &bl) = 0;
-  static LogEvent *decode(bufferlist &bl);
+  virtual void decode(bufferlist::const_iterator &) = 0;
+  static std::unique_ptr<LogEvent> decode_event(bufferlist::const_iterator);
   virtual void dump(Formatter *f) const = 0;
 
   void encode_with_header(bufferlist& bl, uint64_t features) {

diff --git a/src/mds/MDLog.cc b/src/mds/MDLog.cc
index 9c08232f540e38d543df8aa1d4ec74cda7ff03e7..75e5934b31f503d90d6cd47a521662fb63ca9468 100644 (file)
--- a/src/mds/MDLog.cc
+++ b/src/mds/MDLog.cc
@@ -1177,13 +1177,13 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa
     ceph_assert(r);
 
     // Update segment_pos_rewrite
-    LogEvent *le = LogEvent::decode(bl);
+    auto le = LogEvent::decode_event(bl.cbegin());
     if (le) {
       bool modified = false;
 
       if (le->get_type() == EVENT_SUBTREEMAP ||
           le->get_type() == EVENT_RESETJOURNAL) {
-        ESubtreeMap *sle = dynamic_cast<ESubtreeMap*>(le);
+        auto sle = dynamic_cast<ESubtreeMap*>(le.get());
         if (sle == NULL || sle->event_seq == 0) {
           // A non-explicit event seq: the effective sequence number 
           // of this segment is it's position in the old journal and
@@ -1207,11 +1207,10 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa
       // (expire_pos is just an optimization so it's safe to eliminate it)
       if (le->get_type() == EVENT_SUBTREEMAP
           || le->get_type() == EVENT_SUBTREEMAP_TEST) {
-        ESubtreeMap *sle = dynamic_cast<ESubtreeMap*>(le);
-        ceph_assert(sle != NULL);
+        auto& sle = dynamic_cast<ESubtreeMap&>(*le);
         dout(20) << __func__ << " zeroing expire_pos in subtreemap event at "
-          << le_pos << " seq=" << sle->event_seq << dendl;
-        sle->expire_pos = 0;
+          << le_pos << " seq=" << sle.event_seq << dendl;
+        sle.expire_pos = 0;
         modified = true;
       }
 
@@ -1219,8 +1218,6 @@ void MDLog::_reformat_journal(JournalPointer const &jp_in, Journaler *old_journa
         bl.clear();
         le->encode_with_header(bl, mds->mdsmap->get_up_features());
       }
-
-      delete le;
     } else {
       // Failure from LogEvent::decode, our job is to change the journal wrapper,
       // not validate the contents, so pass it through.
@@ -1385,7 +1382,7 @@ void MDLog::_replay_thread()
     ceph_assert(r);
     
     // unpack event
-    LogEvent *le = LogEvent::decode(bl);
+    auto le = LogEvent::decode_event(bl.cbegin());
     if (!le) {
       dout(0) << "_replay " << pos << "~" << bl.length() << " / " << journaler->get_write_pos() 
              << " -- unable to decode event" << dendl;
@@ -1410,7 +1407,7 @@ void MDLog::_replay_thread()
     // new segment?
     if (le->get_type() == EVENT_SUBTREEMAP ||
        le->get_type() == EVENT_RESETJOURNAL) {
-      ESubtreeMap *sle = dynamic_cast<ESubtreeMap*>(le);
+      auto sle = dynamic_cast<ESubtreeMap*>(le.get());
       if (sle && sle->event_seq > 0)
        event_seq = sle->event_seq;
       else
@@ -1442,7 +1439,6 @@ void MDLog::_replay_thread()
         le->replay(mds);
       }
     }
-    delete le;
 
     logger->set(l_mdl_rdpos, pos);
   }

diff --git a/src/tools/cephfs/EventOutput.cc b/src/tools/cephfs/EventOutput.cc
index ed8403f32ab14ab73b0a5c182b2e5dce68ab62af..8cb235a82f987a94238a4cd0d58ea5903f95d7a4 100644 (file)
--- a/src/tools/cephfs/EventOutput.cc
+++ b/src/tools/cephfs/EventOutput.cc
@@ -39,12 +39,10 @@ int EventOutput::binary() const
   for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) {
     bufferlist bin;
     std::stringstream filename;
-    if (i->second.log_event) {
-      LogEvent *le = i->second.log_event;
+    if (auto& le = i->second.log_event; le) {
       le->encode(bin, CEPH_FEATURES_SUPPORTED_DEFAULT);
       filename << "0x" << std::hex << i->first << std::dec << "_" << le->get_type_str() << ".bin";
-    } else if (i->second.pi) {
-      PurgeItem* pi = i->second.pi;
+    } else if (auto& pi = i->second.pi; pi) {
       pi->encode(bin);
       filename << "0x" << std::hex << i->first << std::dec << "_" << pi->get_type_str() << ".bin";
     }
@@ -69,15 +67,11 @@ int EventOutput::json() const
   jf.open_array_section("journal");
   {
     for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) {
-      if (i->second.log_event) {
-       LogEvent *le = i->second.log_event;
+      if (auto& le = i->second.log_event; le) {
        jf.open_object_section("log_event");
-       {
-         le->dump(&jf);
-       }
+       le->dump(&jf);
        jf.close_section();  // log_event
-      } else if (i->second.pi) {
-       PurgeItem* pi = i->second.pi;
+      } else if (auto& pi = i->second.pi; pi) {
        jf.open_object_section("purge_action");
        pi->dump(&jf);
        jf.close_section();
@@ -99,30 +93,30 @@ int EventOutput::json() const
 void EventOutput::list() const
 {
   for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) {
-    if (i->second.log_event) {
+    if (auto& le = i->second.log_event; le) {
       std::vector<std::string> ev_paths;
-      EMetaBlob const *emb = i->second.log_event->get_metablob();
+      EMetaBlob const *emb = le->get_metablob();
       if (emb) {
        emb->get_paths(ev_paths);
       }
 
       std::string detail;
-      if (i->second.log_event->get_type() == EVENT_UPDATE) {
-       EUpdate *eu = reinterpret_cast<EUpdate*>(i->second.log_event);
-       detail = eu->type;
+      if (le->get_type() == EVENT_UPDATE) {
+       auto& eu = reinterpret_cast<EUpdate&>(*le);
+       detail = eu.type;
       }
 
-      std::cout <<i->second.log_event->get_stamp() << " 0x"
+      std::cout << le->get_stamp() << " 0x"
        << std::hex << i->first << std::dec << " "
-       << i->second.log_event->get_type_str() << ": "
+       << le->get_type_str() << ": "
        << " (" << detail << ")" << std::endl;
       for (std::vector<std::string>::iterator i = ev_paths.begin(); i != ev_paths.end(); ++i) {
        std::cout << "  " << *i << std::endl;
       }
-    } else if (i->second.pi) {
-      std::cout << i->second.pi->stamp << " 0x"
+    } else if (auto& pi = i->second.pi; pi) {
+      std::cout << pi->stamp << " 0x"
        << std::hex << i->first << std::dec << " "
-       << i->second.pi->get_type_str() << std::endl;
+       << pi->get_type_str() << std::endl;
     }
   }
 }
@@ -132,10 +126,10 @@ void EventOutput::summary() const
   std::map<std::string, int> type_count;
   for (JournalScanner::EventMap::const_iterator i = scan.events.begin(); i != scan.events.end(); ++i) {
     std::string type;
-    if (i->second.log_event)
-      type = i->second.log_event->get_type_str();
-    else if (i->second.pi)
-      type = i->second.pi->get_type_str();
+    if (auto& le = i->second.log_event; le)
+      type = le->get_type_str();
+    else if (auto& pi = i->second.pi; pi)
+      type = pi->get_type_str();
     if (type_count.count(type) == 0) {
       type_count[type] = 0;
     }

diff --git a/src/tools/cephfs/JournalScanner.cc b/src/tools/cephfs/JournalScanner.cc
index 36c018a4bf5bb52871fcad2bd947c19daf2fa2f3..f81c33d0489db9caf40dc38b855e569834ecc63e 100644 (file)
--- a/src/tools/cephfs/JournalScanner.cc
+++ b/src/tools/cephfs/JournalScanner.cc
@@ -276,15 +276,15 @@ int JournalScanner::scan_events()
         }
         bool valid_entry = true;
         if (is_mdlog) {
-          LogEvent *le = LogEvent::decode(le_bl);
+          auto le = LogEvent::decode_event(le_bl.cbegin());
 
           if (le) {
             dout(10) << "Valid entry at 0x" << std::hex << read_offset << std::dec << dendl;
 
             if (le->get_type() == EVENT_SUBTREEMAP
                 || le->get_type() == EVENT_SUBTREEMAP_TEST) {
-              ESubtreeMap *sle = dynamic_cast<ESubtreeMap*>(le);
-              if (sle->expire_pos > read_offset) {
+              auto&& sle = dynamic_cast<ESubtreeMap&>(*le);
+              if (sle.expire_pos > read_offset) {
                 errors.insert(std::make_pair(
                       read_offset, EventError(
                         -ERANGE,
@@ -293,22 +293,18 @@ int JournalScanner::scan_events()
             }
 
             if (filter.apply(read_offset, *le)) {
-              events[read_offset] = EventRecord(le, consumed);
-            } else {
-              delete le;
+              events.insert_or_assign(read_offset, EventRecord(std::move(le), consumed));
             }
           } else {
             valid_entry = false;
           }
         } else if (type == "purge_queue"){
-           PurgeItem* pi = new PurgeItem();
+           auto pi = std::make_unique<PurgeItem>();
            try {
              auto q = le_bl.cbegin();
              pi->decode(q);
             if (filter.apply(read_offset, *pi)) {
-              events[read_offset] = EventRecord(pi, consumed);
-            } else {
-              delete pi;
+              events.insert_or_assign(read_offset, EventRecord(std::move(pi), consumed));
             }
            } catch (const buffer::error &err) {
              valid_entry = false;
@@ -351,12 +347,6 @@ JournalScanner::~JournalScanner()
     header = NULL;
   }
   dout(4) << events.size() << " events" << dendl;
-  for (EventMap::iterator i = events.begin(); i != events.end(); ++i) {
-    if (i->second.log_event)
-      delete i->second.log_event;
-    else if (i->second.pi)
-      delete i->second.pi;
-  }
   events.clear();
 }
 

diff --git a/src/tools/cephfs/JournalScanner.h b/src/tools/cephfs/JournalScanner.h
index 89ee2fd2803f4efae90e3a4934c0b053727fee7b..018a3b30fd2988309ddc54d3a484fc952ad02c2f 100644 (file)
--- a/src/tools/cephfs/JournalScanner.h
+++ b/src/tools/cephfs/JournalScanner.h
@@ -87,14 +87,12 @@ class JournalScanner
 
   // The results of the scan
   inodeno_t ino;  // Corresponds to journal ino according their type
-  class EventRecord {
-    public:
-    EventRecord() : log_event(NULL), raw_size(0) {}
-    EventRecord(LogEvent *le, uint32_t rs) : log_event(le), pi(NULL), raw_size(rs) {}
-    EventRecord(PurgeItem* p, uint32_t rs) : log_event(NULL), pi(p), raw_size(rs) {}
-    LogEvent *log_event;
-    PurgeItem *pi;
-    uint32_t raw_size;  //< Size from start offset including all encoding overhead
+  struct EventRecord {
+    EventRecord(std::unique_ptr<LogEvent> le, uint32_t rs) : log_event(std::move(le)), raw_size(rs) {}
+    EventRecord(std::unique_ptr<PurgeItem> p, uint32_t rs) : pi(std::move(p)), raw_size(rs) {}
+    std::unique_ptr<LogEvent> log_event;
+    std::unique_ptr<PurgeItem> pi;
+    uint32_t raw_size = 0;  //< Size from start offset including all encoding overhead
   };
 
   class EventError {

diff --git a/src/tools/cephfs/JournalTool.cc b/src/tools/cephfs/JournalTool.cc
index 37f3b5e165a5cdbf3779a8dfbf4c721c97c6c242..29dca56df00c24cc69f00beea8ef1b72f25267c7 100644 (file)
--- a/src/tools/cephfs/JournalTool.cc
+++ b/src/tools/cephfs/JournalTool.cc
@@ -452,7 +452,7 @@ int JournalTool::main_event(std::vector<const char*> &argv)
     std::set<inodeno_t> consumed_inos;
     for (JournalScanner::EventMap::iterator i = js.events.begin();
          i != js.events.end(); ++i) {
-      LogEvent *le = i->second.log_event;
+      auto& le = i->second.log_event;
       EMetaBlob const *mb = le->get_metablob();
       if (mb) {
         int scav_r = recover_dentries(*mb, dry_run, &consumed_inos);