osd: separate filter init from construction

...so that implementations can readily handle
decode errors in client args and return an
error code.

Signed-off-by: John Spray <john.spray@redhat.com>

src/cls/hello/cls_hello.cc

@@ -261,10 +261,15 @@ static int bad_writer(cls_method_context_t hctx, bufferlist *in, bufferlist *out
 class PGLSHelloFilter : public PGLSFilter {
   string val;
 public:
-  PGLSHelloFilter(bufferlist::iterator& params) {
-    ::decode(xattr, params);
-    ::decode(val, params);
+  int init(bufferlist::iterator& params) {
+    try {
+      ::decode(xattr, params);
+      ::decode(val, params);
+    } catch (buffer::error &e) {
+      return -EINVAL;
+    }
   }
+
   virtual ~PGLSHelloFilter() {}
   virtual bool filter(const hobject_t &obj, bufferlist& xattr_data,
                       bufferlist& outdata)
@@ -280,10 +285,9 @@ public:
 };
 
 
-PGLSFilter *hello_filter(bufferlist::iterator *q)
+PGLSFilter *hello_filter()
 {
-  assert(q);
-  return new PGLSHelloFilter(*q);
+  return new PGLSHelloFilter();
 }
 
 

src/objclass/objclass.h

@@ -106,6 +106,13 @@ public:
   virtual bool filter(const hobject_t &obj, bufferlist& xattr_data,
                       bufferlist& outdata) = 0;
 
+  /**
+   * Arguments passed from the RADOS client.  Implementations must
+   * handle any encoding errors, and return an appropriate error code,
+   * or 0 on valid input.
+   */
+  virtual int init(bufferlist::iterator &params) = 0;
+
   /**
    * xattr key, or empty string.  If non-empty, this xattr will be fetched
    * and the value passed into ::filter
@@ -120,9 +127,7 @@ public:
 };
 
 // Classes expose a filter constructor that returns a subclass of PGLSFilter
-typedef PGLSFilter* (*cls_cxx_filter_factory_t)(
-    bufferlist::iterator *args);
-
+typedef PGLSFilter* (*cls_cxx_filter_factory_t)();
 
 
 extern int cls_register_cxx_method(cls_handle_t hclass, const char *method, int flags,

src/osd/ReplicatedPG.cc

@@ -512,9 +512,16 @@ void ReplicatedPG::wait_for_blocked_object(const hobject_t& soid, OpRequestRef o
 class PGLSPlainFilter : public PGLSFilter {
   string val;
 public:
-  PGLSPlainFilter(bufferlist::iterator& params) {
-    ::decode(xattr, params);
-    ::decode(val, params);
+  virtual int init(bufferlist::iterator &params)
+  {
+    try {
+      ::decode(xattr, params);
+      ::decode(val, params);
+    } catch (buffer::error &e) {
+      return -EINVAL;
+    }
+
+    return 0;
   }
   virtual ~PGLSPlainFilter() {}
   virtual bool filter(const hobject_t &obj, bufferlist& xattr_data,
@@ -524,10 +531,19 @@ public:
 class PGLSParentFilter : public PGLSFilter {
   inodeno_t parent_ino;
 public:
-  PGLSParentFilter(bufferlist::iterator& params) {
+  PGLSParentFilter() {
     xattr = "_parent";
-    ::decode(parent_ino, params);
+  }
+  virtual int init(bufferlist::iterator &params)
+  {
+    try {
+      ::decode(parent_ino, params);
+    } catch (buffer::error &e) {
+      return -EINVAL;
+    }
     generic_dout(0) << "parent_ino=" << parent_ino << dendl;
+
+    return 0;
   }
   virtual ~PGLSParentFilter() {}
   virtual bool filter(const hobject_t &obj, bufferlist& xattr_data,
@@ -602,9 +618,9 @@ int ReplicatedPG::get_pgls_filter(bufferlist::iterator& iter, PGLSFilter **pfilt
   }
 
   if (type.compare("parent") == 0) {
-    filter = new PGLSParentFilter(iter);
+    filter = new PGLSParentFilter();
   } else if (type.compare("plain") == 0) {
-    filter = new PGLSPlainFilter(iter);
+    filter = new PGLSPlainFilter();
   } else {
     std::size_t dot = type.find(".");
     if (dot == std::string::npos || dot == 0 || dot == type.size() - 1) {
@@ -629,13 +645,28 @@ int ReplicatedPG::get_pgls_filter(bufferlist::iterator& iter, PGLSFilter **pfilt
            << class_name << dendl;
       return -EINVAL;
     }
-    filter = class_filter->fn(&iter);
-    assert(filter);
+    filter = class_filter->fn();
+    if (!filter) {
+      // Object classes are obliged to return us something, but let's
+      // give an error rather than asserting out.
+      derr << "Buggy class " << class_name << " failed to construct "
+              "filter " << filter_name << dendl;
+      return -EINVAL;
+    }
   }
 
-  *pfilter = filter;
-
-  return  0;
+  assert(filter);
+  int r = filter->init(iter);
+  if (r < 0) {
+    derr << "Error initializing filter " << type << ": "
+         << cpp_strerror(r) << dendl;
+    delete filter;
+    return -EINVAL;
+  } else {
+    // Successfully constructed and initialized, return it.
+    *pfilter = filter;
+    return  0;
+  }
 }