From: Patrick Donnelly <pdonnell@redhat.com>
Date: Fri, 11 Jan 2019 20:28:42 +0000 (-0800)
Subject: common/TrackedOp: use string_view interface
X-Git-Tag: v14.1.0~390^2~1
X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=e62b3356227d08d3ce83382d3f3664fe88570a43;p=ceph-ci.git

common/TrackedOp: use string_view interface

Also, don't store pointers to strings outside. The interface was written to
optimize for string literals but this is structurally unsafe programming.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
---

diff --git a/src/common/TrackedOp.cc b/src/common/TrackedOp.cc
index b561101afd6..975e58a2260 100644
--- a/src/common/TrackedOp.cc
+++ b/src/common/TrackedOp.cc
@@ -390,7 +390,7 @@ bool OpTracker::check_ops_in_flight(std::string* summary,
     ss << "slow request " << age << " seconds old, received at "
        << op.get_initiated() << ": " << op.get_desc()
        << " currently "
-        << (op.current ? op.current : op.state_string());
+       << op.state_string();
     warnings.push_back(ss.str());
     // only those that have been shown will backoff
     op.warn_interval_multiplier *= 2;
@@ -434,7 +434,7 @@ void OpTracker::get_age_ms_histogram(pow2_hist_t *h)
 #undef dout_context
 #define dout_context tracker->cct
 
-void TrackedOp::mark_event_string(const string &event, utime_t stamp)
+void TrackedOp::mark_event(std::string_view event, utime_t stamp)
 {
   if (!state)
     return;
@@ -442,25 +442,6 @@ void TrackedOp::mark_event_string(const string &event, utime_t stamp)
   {
     std::lock_guard l(lock);
     events.emplace_back(stamp, event);
-    current = events.back().c_str();
-  }
-  dout(6) << " seq: " << seq
-	  << ", time: " << stamp
-	  << ", event: " << event
-	  << ", op: " << get_desc()
-	  << dendl;
-  _event_marked();
-}
-
-void TrackedOp::mark_event(const char *event, utime_t stamp)
-{
-  if (!state)
-    return;
-
-  {
-    std::lock_guard l(lock);
-    events.emplace_back(stamp, event);
-    current = event;
   }
   dout(6) << " seq: " << seq
 	  << ", time: " << stamp
diff --git a/src/common/TrackedOp.h b/src/common/TrackedOp.h
index 8c6c37b9e9e..71072070bbe 100644
--- a/src/common/TrackedOp.h
+++ b/src/common/TrackedOp.h
@@ -234,35 +234,26 @@ protected:
 
   struct Event {
     utime_t stamp;
-    string str;
-    const char *cstr = nullptr;
+    std::string str;
 
-    Event(utime_t t, const string& s) : stamp(t), str(s) {}
-    Event(utime_t t, const char *s) : stamp(t), cstr(s) {}
+    Event(utime_t t, std::string_view s) : stamp(t), str(s) {}
 
     int compare(const char *s) const {
-      if (cstr)
-	return strcmp(cstr, s);
-      else
-	return str.compare(s);
+      return str.compare(s);
     }
 
     const char *c_str() const {
-      if (cstr)
-	return cstr;
-      else
-	return str.c_str();
+      return str.c_str();
     }
 
     void dump(Formatter *f) const {
       f->dump_stream("time") << stamp;
-      f->dump_string("event", c_str());
+      f->dump_string("event", str);
     }
   };
 
   vector<Event> events;    ///< list of events and their times
   mutable ceph::mutex lock = ceph::make_mutex("TrackedOp::lock"); ///< to protect the events list
-  const char *current = 0; ///< the current state the event is in
   uint64_t seq = 0;        ///< a unique value set by the OpTracker
 
   uint32_t warn_interval_multiplier = 1; //< limits output of a given op warning
@@ -374,18 +365,15 @@ public:
       return ceph_clock_now() - get_initiated();
   }
 
-  void mark_event_string(const string &event,
-			 utime_t stamp=ceph_clock_now());
-  void mark_event(const char *event,
-		  utime_t stamp=ceph_clock_now());
+  void mark_event(std::string_view event, utime_t stamp=ceph_clock_now());
 
   void mark_nowarn() {
     warn_interval_multiplier = 0;
   }
 
-  virtual const char *state_string() const {
+  virtual std::string_view state_string() const {
     std::lock_guard l(lock);
-    return events.rbegin()->c_str();
+    return events.empty() ? std::string_view() : std::string_view(events.rbegin()->str);
   }
 
   void dump(utime_t now, Formatter *f) const;
diff --git a/src/mds/Server.cc b/src/mds/Server.cc
index 0171b3b2eb3..5188c3d2a6b 100644
--- a/src/mds/Server.cc
+++ b/src/mds/Server.cc
@@ -1599,12 +1599,12 @@ void Server::journal_and_reply(MDRequestRef& mdr, CInode *in, CDentry *dn, LogEv
 }
 
 void Server::submit_mdlog_entry(LogEvent *le, MDSLogContextBase *fin, MDRequestRef& mdr,
-                                const char *event)
+                                std::string_view event)
 {
   if (mdr) {
     string event_str("submit entry: ");
     event_str += event;
-    mdr->mark_event_string(event_str);
+    mdr->mark_event(event_str);
   } 
   mdlog->submit_entry(le, fin);
 }
diff --git a/src/mds/Server.h b/src/mds/Server.h
index dbd82ef3e3e..d49d653abfe 100644
--- a/src/mds/Server.h
+++ b/src/mds/Server.h
@@ -173,7 +173,7 @@ public:
   void journal_and_reply(MDRequestRef& mdr, CInode *tracei, CDentry *tracedn,
 			 LogEvent *le, MDSLogContextBase *fin);
   void submit_mdlog_entry(LogEvent *le, MDSLogContextBase *fin,
-                          MDRequestRef& mdr, const char *evt);
+                          MDRequestRef& mdr, std::string_view event);
   void dispatch_client_request(MDRequestRef& mdr);
   void perf_gather_op_latency(const MClientRequest::const_ref &req, utime_t lat);
   void early_reply(MDRequestRef& mdr, CInode *tracei, CDentry *tracedn);
diff --git a/src/mon/MonOpRequest.h b/src/mon/MonOpRequest.h
index 5c1d6eeacc7..6b0fdf3e49b 100644
--- a/src/mon/MonOpRequest.h
+++ b/src/mon/MonOpRequest.h
@@ -41,7 +41,7 @@ struct MonOpRequest : public TrackedOp {
   void mark_svc_event(const string &service, const string &event) {
     string s = service;
     s.append(":").append(event);
-    mark_event_string(s);
+    mark_event(s);
   }
 
   void mark_logmon_event(const string &event) {
@@ -217,7 +217,7 @@ struct C_MonOp : public Context
 
   void mark_op_event(const string &event) {
     if (op)
-      op->mark_event_string(event);
+      op->mark_event(event);
   }
 
   virtual void _finish(int r) = 0;
diff --git a/src/mon/PaxosService.h b/src/mon/PaxosService.h
index d1b96b9a191..36d39ad799b 100644
--- a/src/mon/PaxosService.h
+++ b/src/mon/PaxosService.h
@@ -543,7 +543,7 @@ public:
    */
   void wait_for_finished_proposal(MonOpRequestRef op, Context *c) {
     if (op)
-      op->mark_event_string(service_name + ":wait_for_finished_proposal");
+      op->mark_event(service_name + ":wait_for_finished_proposal");
     waiting_for_finished_proposal.push_back(c);
   }
   void wait_for_finished_proposal_ctx(Context *c) {
@@ -558,7 +558,7 @@ public:
    */
   void wait_for_active(MonOpRequestRef op, Context *c) {
     if (op)
-      op->mark_event_string(service_name + ":wait_for_active");
+      op->mark_event(service_name + ":wait_for_active");
 
     if (!is_proposing()) {
       paxos->wait_for_active(op, c);
@@ -585,7 +585,7 @@ public:
      * happens to be readable at that specific point in time.
      */
     if (op)
-      op->mark_event_string(service_name + ":wait_for_readable");
+      op->mark_event(service_name + ":wait_for_readable");
 
     if (is_proposing() ||
 	ver > get_last_committed() ||
@@ -593,7 +593,7 @@ public:
       wait_for_finished_proposal(op, c);
     else {
       if (op)
-        op->mark_event_string(service_name + ":wait_for_readable/paxos");
+        op->mark_event(service_name + ":wait_for_readable/paxos");
 
       paxos->wait_for_readable(op, c);
     }
@@ -611,7 +611,7 @@ public:
    */
   void wait_for_writeable(MonOpRequestRef op, Context *c) {
     if (op)
-      op->mark_event_string(service_name + ":wait_for_writeable");
+      op->mark_event(service_name + ":wait_for_writeable");
 
     if (is_proposing())
       wait_for_finished_proposal(op, c);
diff --git a/src/osd/OpRequest.cc b/src/osd/OpRequest.cc
index a33c1391abe..35ff7f28d9e 100644
--- a/src/osd/OpRequest.cc
+++ b/src/osd/OpRequest.cc
@@ -151,7 +151,7 @@ void OpRequest::mark_flag_point_string(uint8_t flag, const string& s) {
 #ifdef WITH_LTTNG
   uint8_t old_flags = hit_flag_points;
 #endif
-  mark_event_string(s);
+  mark_event(s);
   hit_flag_points |= flag;
   latest_flag_point = flag;
   tracepoint(oprequest, mark_flag_point, reqid.name._type,
diff --git a/src/osd/OpRequest.h b/src/osd/OpRequest.h
index 5bfb730f5d0..184d26acc29 100644
--- a/src/osd/OpRequest.h
+++ b/src/osd/OpRequest.h
@@ -121,7 +121,7 @@ public:
     }
   }
 
-  const char *state_string() const override {
+  std::string_view state_string() const override {
     switch(latest_flag_point) {
     case flag_queued_for_pg: return "queued for pg";
     case flag_reached_pg: return "reached pg";
diff --git a/src/osd/ReplicatedBackend.cc b/src/osd/ReplicatedBackend.cc
index a06c4f4448e..18532fd9687 100644
--- a/src/osd/ReplicatedBackend.cc
+++ b/src/osd/ReplicatedBackend.cc
@@ -566,7 +566,7 @@ void ReplicatedBackend::do_repop_reply(OpRequestRef op)
       ceph_assert(ip_op.waiting_for_commit.count(from));
       ip_op.waiting_for_commit.erase(from);
       if (ip_op.op) {
-	ip_op.op->mark_event_string("sub_op_commit_rec");
+	ip_op.op->mark_event("sub_op_commit_rec");
 	ip_op.op->pg_trace.event("sub_op_commit_rec");
       }
     } else {