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>

diff --git a/src/messages/MForward.h b/src/messages/MForward.h
index fffee64d89471f8820c2fc6f83aa03b5e70553b2..514c4a359557abb8a675b79d6890b72d67bf751f 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, features);
     ::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 << ")";
   }
 };