messages/MForward: fix encoding features

We were encoding the message with the sending client's
features, which makes no sense: we need to encode with
the recipient's features so that it can decode the
message.

The simplest way to fix this is to rip out the bizarre
msg_bl handling code and simply keep a decoded Message
reference, and encode it when we send.

We encode the encapsulated message with the intersection
of the target mon's features and the sending client's
features.  This probably doesn't matter, but it's
conceivable that there is some feature-dependent
behavior in the message encode/decode that is important.

Fixes: http://tracker.ceph.com/issues/17365
Signed-off-by: Sage Weil <sage@redhat.com>
(cherry picked from commit d4f5e88f36e5388ae9e062c4bc49ac1c684a3f3c)

diff --git a/src/messages/MForward.h b/src/messages/MForward.h
index efe608563cd77029aa4c6a93f6c19ce0e284d580..c6804cd55cba59cfa6e63a9d9df1f2cac3f2dc98 100644 (file)
--- a/src/messages/MForward.h
+++ b/src/messages/MForward.h
@@ -22,6 +22,7 @@
 #include "msg/Message.h"
 #include "mon/MonCap.h"
 #include "include/encoding.h"
+#include "include/stringify.h"
 
 struct MForward : public Message {
   uint64_t tid;
@@ -29,9 +30,10 @@ struct MForward : public Message {
   MonCap client_caps;
   uint64_t con_features;
   EntityName entity_name;
-  PaxosServiceMessage *msg;   // incoming message
-  bufferlist msg_bl;          // outgoing message
+  PaxosServiceMessage *msg;   // incoming or outgoing message
 
+  string msg_desc;  // for operator<< only
+  
   static const int HEAD_VERSION = 3;
   static const int COMPAT_VERSION = 1;
 
@@ -44,7 +46,9 @@ struct MForward : public Message {
     client = m->get_source_inst();
     client_caps = m->get_session()->caps;
     con_features = feat;
-    set_message(m, feat);
+    // we may need to reencode for the target mon
+    msg->clear_payload();
+    msg = (PaxosServiceMessage*)m->get();
   }
   MForward(uint64_t t, PaxosServiceMessage *m, uint64_t feat,
            const MonCap& caps) :
@@ -52,7 +56,7 @@ struct MForward : public Message {
     tid(t), client_caps(caps), msg(NULL) {
     client = m->get_source_inst();
     con_features = feat;
-    set_message(m, feat);
+    msg = (PaxosServiceMessage*)m->get();
   }
 private:
   ~MForward() {
@@ -63,18 +67,17 @@ private:
     }
   }
 
-  PaxosServiceMessage *get_msg_from_bl() {
-    bufferlist::iterator p = msg_bl.begin();
-    return (msg_bl.length() ?
-        (PaxosServiceMessage*)decode_message(NULL, 0, p) : NULL);
-  }
-
 public:
   void encode_payload(uint64_t features) {
     ::encode(tid, payload);
     ::encode(client, payload);
     ::encode(client_caps, payload, features);
-    payload.append(msg_bl);
+    // Encode client message with intersection of target and source
+    // features.  This could matter if the semantics of the encoded
+    // message are changed when reencoding with more features than the
+    // client had originally.  That should never happen, but we may as
+    // well be defensive here.
+    encode_message(msg, features & con_features, payload);
     ::encode(con_features, payload);
     ::encode(entity_name, payload);
   }
@@ -101,21 +104,10 @@ public:
 
   }
 
-  void set_message(PaxosServiceMessage *m, uint64_t features) {
-    // get a reference to the message.  We will not use it except for print(),
-    // and we will put it in the dtor if it is not claimed.
-    // we could avoid doing this if only we had a const bufferlist iterator :)
-    msg = (PaxosServiceMessage*)m->get();
-
-    encode_message(m, features, msg_bl);
-  }
-
   PaxosServiceMessage *claim_message() {
-    if (!msg) {
-      return get_msg_from_bl();
-    }
-
     // let whoever is claiming the message deal with putting it.
+    assert(msg);
+    msg_desc = stringify(*msg);
     PaxosServiceMessage *m = msg;
     msg = NULL;
     return m;
@@ -123,13 +115,15 @@ public:
 
   const char *get_type_name() const { return "forward"; }
   void print(ostream& o) const {
+    o << "forward(";
     if (msg) {
-      o << "forward(" << *msg << " caps " << client_caps
-       << " tid " << tid
-        << " con_features " << con_features << ") to leader";
+      o << *msg;
     } else {
-      o << "forward(??? ) to leader";
+      o << msg_desc;
     }
+    o << " caps " << client_caps
+      << " tid " << tid
+      << " con_features " << con_features << ")";
   }
 };