rgw: lifecycle and xml parser fixes

Signed-off-by: Yehuda Sadeh <yehuda@redhat.com>

diff --git a/src/rgw/rgw_lc.cc b/src/rgw/rgw_lc.cc
index 2fed865357adaee62a30c2908b877751f1a8af9e..ba2f9b78477ecb89febcba2db095b8ab302cbbd1 100644 (file)
--- a/src/rgw/rgw_lc.cc
+++ b/src/rgw/rgw_lc.cc
@@ -31,7 +31,7 @@ const char* LC_STATUS[] = {
 
 using namespace librados;
 
-bool LCRule::valid()
+bool LCRule::valid() const
 {
   if (id.length() > MAX_ID_LEN) {
     return false;
@@ -76,31 +76,30 @@ void LCRule::init_simple_days_rule(std::string_view _id, std::string_view _prefi
   set_enabled(true);
 }
 
-void RGWLifecycleConfiguration::add_rule(LCRule *rule)
+void RGWLifecycleConfiguration::add_rule(const LCRule& rule)
 {
-  string id;
-  rule->get_id(id); // note that this will return false for groups, but that's ok, we won't search groups
-  rule_map.insert(pair<string, LCRule>(id, *rule));
+  auto& id = rule.get_id(); // note that this will return false for groups, but that's ok, we won't search groups
+  rule_map.insert(pair<string, LCRule>(id, rule));
 }
 
-bool RGWLifecycleConfiguration::_add_rule(LCRule *rule)
+bool RGWLifecycleConfiguration::_add_rule(const LCRule& rule)
 {
   lc_op op;
-  op.status = rule->is_enabled();
-  if (rule->get_expiration().has_days()) {
-    op.expiration = rule->get_expiration().get_days();
+  op.status = rule.is_enabled();
+  if (rule.get_expiration().has_days()) {
+    op.expiration = rule.get_expiration().get_days();
   }
-  if (rule->get_expiration().has_date()) {
-    op.expiration_date = ceph::from_iso_8601(rule->get_expiration().get_date());
+  if (rule.get_expiration().has_date()) {
+    op.expiration_date = ceph::from_iso_8601(rule.get_expiration().get_date());
   }
-  if (rule->get_noncur_expiration().has_days()) {
-    op.noncur_expiration = rule->get_noncur_expiration().get_days();
+  if (rule.get_noncur_expiration().has_days()) {
+    op.noncur_expiration = rule.get_noncur_expiration().get_days();
   }
-  if (rule->get_mp_expiration().has_days()) {
-    op.mp_expiration = rule->get_mp_expiration().get_days();
+  if (rule.get_mp_expiration().has_days()) {
+    op.mp_expiration = rule.get_mp_expiration().get_days();
   }
-  op.dm_expiration = rule->get_dm_expiration();
-  for (const auto &elem : rule->get_transitions()) {
+  op.dm_expiration = rule.get_dm_expiration();
+  for (const auto &elem : rule.get_transitions()) {
     transition_action action;
     if (elem.second.has_days()) {
       action.days = elem.second.get_days();
@@ -110,7 +109,7 @@ bool RGWLifecycleConfiguration::_add_rule(LCRule *rule)
     action.storage_class = elem.first;
     op.transitions.emplace(elem.first, std::move(action));
   }
-  for (const auto &elem : rule->get_noncur_transitions()) {
+  for (const auto &elem : rule.get_noncur_transitions()) {
     transition_action action;
     action.days = elem.second.get_days();
     action.date = ceph::from_iso_8601(elem.second.get_date());
@@ -118,30 +117,29 @@ bool RGWLifecycleConfiguration::_add_rule(LCRule *rule)
     op.noncur_transitions.emplace(elem.first, std::move(action));
   }
   std::string prefix;
-  if (rule->get_filter().has_prefix()){
-    prefix = rule->get_filter().get_prefix();
+  if (rule.get_filter().has_prefix()){
+    prefix = rule.get_filter().get_prefix();
   } else {
-    prefix = rule->get_prefix();
+    prefix = rule.get_prefix();
   }
 
-  if (rule->get_filter().has_tags()){
-    op.obj_tags = rule->get_filter().get_tags();
+  if (rule.get_filter().has_tags()){
+    op.obj_tags = rule.get_filter().get_tags();
   }
   auto ret = prefix_map.emplace(std::move(prefix), std::move(op));
   return ret.second;
 }
 
-int RGWLifecycleConfiguration::check_and_add_rule(LCRule *rule)
+int RGWLifecycleConfiguration::check_and_add_rule(const LCRule& rule)
 {
-  if (!rule->valid()) {
+  if (!rule.valid()) {
     return -EINVAL;
   }
-  string id;
-  rule->get_id(id);
+  auto& id = rule.get_id();
   if (rule_map.find(id) != rule_map.end()) {  //id shouldn't be the same 
     return -EINVAL;
   }
-  rule_map.insert(pair<string, LCRule>(id, *rule));
+  rule_map.insert(pair<string, LCRule>(id, rule));
 
   if (!_add_rule(rule)) {
     return -ERR_INVALID_REQUEST;

diff --git a/src/rgw/rgw_lc.h b/src/rgw/rgw_lc.h
index 21fd5d5b15165965fc2e2ce509114d7d07811fd4..80cb220294febe6c3fa9aaf52e5efb03dce495b6 100644 (file)
--- a/src/rgw/rgw_lc.h
+++ b/src/rgw/rgw_lc.h
@@ -232,16 +232,15 @@ public:
   LCRule(){};
   ~LCRule(){};
 
-  bool get_id(string& _id) {
-      _id = id;
-      return true;
+  const string& get_id() const {
+      return id;
   }
 
-  string& get_status() {
+  const string& get_status() const {
       return status;
   }
 
-  bool is_enabled() {
+  bool is_enabled() const {
     return status == "Enabled";
   }
 
@@ -249,35 +248,35 @@ public:
     status = (flag ? "Enabled" : "Disabled");
   }
 
-  string& get_prefix() {
+  const string& get_prefix() const {
       return prefix;
   }
 
-  LCFilter& get_filter() {
+  const LCFilter& get_filter() const {
     return filter;
   }
 
-  LCExpiration& get_expiration() {
+  const LCExpiration& get_expiration() const {
     return expiration;
   }
 
-  LCExpiration& get_noncur_expiration() {
+  const LCExpiration& get_noncur_expiration() const {
     return noncur_expiration;
   }
 
-  LCExpiration& get_mp_expiration() {
+  const LCExpiration& get_mp_expiration() const {
     return mp_expiration;
   }
 
-  bool get_dm_expiration() {
+  bool get_dm_expiration() const {
     return dm_expiration;
   }
 
-  map<string, LCTransition>& get_transitions() {
+  const map<string, LCTransition>& get_transitions() const {
     return transitions;
   }
 
-  map<string, LCTransition>& get_noncur_transitions() {
+  const map<string, LCTransition>& get_noncur_transitions() const {
     return noncur_transitions;
   }
 
@@ -309,17 +308,17 @@ public:
     dm_expiration = _dm_expiration;
   }
 
-  bool add_transition(LCTransition* _transition) {
-    auto ret = transitions.emplace(_transition->get_storage_class(), *_transition);
+  bool add_transition(const LCTransition& _transition) {
+    auto ret = transitions.emplace(_transition.get_storage_class(), _transition);
     return ret.second;
   }
 
-  bool add_noncur_transition(LCTransition* _noncur_transition) {
-    auto ret = noncur_transitions.emplace(_noncur_transition->get_storage_class(), *_noncur_transition);
+  bool add_noncur_transition(const LCTransition& _noncur_transition) {
+    auto ret = noncur_transitions.emplace(_noncur_transition.get_storage_class(), _noncur_transition);
     return ret.second;
   }
 
-  bool valid();
+  bool valid() const;
   
   void encode(bufferlist& bl) const {
      ENCODE_START(6, 1, bl);
@@ -394,7 +393,7 @@ protected:
   CephContext *cct;
   map<string, lc_op> prefix_map;
   multimap<string, LCRule> rule_map;
-  bool _add_rule(LCRule *rule);
+  bool _add_rule(const LCRule& rule);
   bool has_same_action(const lc_op& first, const lc_op& second);
 public:
   explicit RGWLifecycleConfiguration(CephContext *_cct) : cct(_cct) {}
@@ -419,16 +418,16 @@ public:
     multimap<string, LCRule>::iterator iter;
     for (iter = rule_map.begin(); iter != rule_map.end(); ++iter) {
       LCRule& rule = iter->second;
-      _add_rule(&rule);
+      _add_rule(rule);
     }
     DECODE_FINISH(bl);
   }
   void dump(Formatter *f) const;
   static void generate_test_instances(list<RGWLifecycleConfiguration*>& o);
 
-  void add_rule(LCRule* rule);
+  void add_rule(const LCRule& rule);
 
-  int check_and_add_rule(LCRule* rule);
+  int check_and_add_rule(const LCRule& rule);
 
   bool valid();
 

diff --git a/src/rgw/rgw_lc_s3.cc b/src/rgw/rgw_lc_s3.cc
index c175e2ef54fa029d80d48750480a76384f138260..144a92f4cbd592d813d972056563becce04b6c11 100644 (file)
--- a/src/rgw/rgw_lc_s3.cc
+++ b/src/rgw/rgw_lc_s3.cc
@@ -44,8 +44,9 @@ void LCExpiration_S3::decode_xml(XMLObj *obj)
   string dm;
   bool has_dm = RGWXMLDecoder::decode_xml("ExpiredObjectDeleteMarker", dm, obj);
 
-  if ((!has_days && !has_dm && !has_date) || (has_days && has_dm) 
-      || (has_days && has_date) || (has_dm && has_date)) {
+  int num = !!has_days + !!has_date + !!has_dm;
+
+  if (num != 1) {
     throw RGWXMLDecoder::err("bad Expiration section");
   }
 
@@ -53,6 +54,10 @@ void LCExpiration_S3::decode_xml(XMLObj *obj)
     //We need return xml error according to S3
     throw RGWXMLDecoder::err("bad date in Date section");
   }
+
+  if (has_dm) {
+    dm_expiration = (dm == "true");
+  }
 }
 
 void LCNoncurExpiration_S3::decode_xml(XMLObj *obj)
@@ -77,12 +82,25 @@ void LCMPExpiration_S3::dump_xml(Formatter *f) const
 
 void RGWLifecycleConfiguration_S3::decode_xml(XMLObj *obj)
 {
+  if (!cct) {
+    throw RGWXMLDecoder::err("ERROR: RGWLifecycleConfiguration_S3 can't be decoded without cct initialized");
+  }
   vector<LCRule_S3> rules;
 
   RGWXMLDecoder::decode_xml("Rule", rules, obj, true);
 
   for (auto& rule : rules) {
-    add_rule(&rule);
+    if (rule.get_id().empty()) {
+      string id;
+
+      // S3 generates a 48 bit random ID, maybe we could generate shorter IDs
+      static constexpr auto LC_ID_LENGTH = 48;
+
+      gen_rand_alphanumeric_lower(cct, &id, LC_ID_LENGTH);
+      rule.set_id(id);
+    }
+
+    add_rule(rule);
   }
 
   if (cct->_conf->rgw_lc_max_rules < rule_map.size()) {
@@ -192,12 +210,7 @@ void LCRule_S3::decode_xml(XMLObj *obj)
   status.clear();
   dm_expiration = false;
 
-  // S3 generates a 48 bit random ID, maybe we could generate shorter IDs
-  static constexpr auto LC_ID_LENGTH = 48;
-
-  if (!RGWXMLDecoder::decode_xml("ID", id, obj)) {
-    gen_rand_alphanumeric_lower(cct, &id, LC_ID_LENGTH);
-  }
+  RGWXMLDecoder::decode_xml("ID", id, obj);
 
   LCFilter_S3 filter_s3;
   if (!RGWXMLDecoder::decode_xml("Filter", filter_s3, obj)) {
@@ -222,10 +235,9 @@ void LCRule_S3::decode_xml(XMLObj *obj)
     throw RGWXMLDecoder::err("bad Status in Filter");
   }
 
-
   LCExpiration_S3 s3_expiration;
-  LCExpiration_S3 s3_noncur_expiration;
-  LCExpiration_S3 s3_mp_expiration;
+  LCNoncurExpiration_S3 s3_noncur_expiration;
+  LCMPExpiration_S3 s3_mp_expiration;
   LCFilter_S3 s3_filter;
 
   bool has_expiration = RGWXMLDecoder::decode_xml("Expiration", s3_expiration, obj);
@@ -261,12 +273,12 @@ void LCRule_S3::decode_xml(XMLObj *obj)
     mp_expiration = s3_mp_expiration;
   }
   for (auto& t : transitions) {
-    if (!add_transition(&t)) {
+    if (!add_transition(t)) {
       throw RGWXMLDecoder::err("Failed to add transition");
     }
   }
   for (auto& t : noncur_transitions) {
-    if (!add_noncur_transition(&t)) {
+    if (!add_noncur_transition(t)) {
       throw RGWXMLDecoder::err("Failed to add non-current version transition");
     }
   }
@@ -314,7 +326,7 @@ int RGWLifecycleConfiguration_S3::rebuild(RGWRados *store, RGWLifecycleConfigura
   multimap<string, LCRule>::iterator iter;
   for (iter = rule_map.begin(); iter != rule_map.end(); ++iter) {
     LCRule& src_rule = iter->second;
-    ret = dest.check_and_add_rule(&src_rule);
+    ret = dest.check_and_add_rule(src_rule);
     if (ret < 0)
       return ret;
   }

diff --git a/src/rgw/rgw_lc_s3.h b/src/rgw/rgw_lc_s3.h
index 8fbeba8df732414b68e0747ad641d342b821cf99..214ca54c55e523dbdd03cc069057e1512f6f68e2 100644 (file)
--- a/src/rgw/rgw_lc_s3.h
+++ b/src/rgw/rgw_lc_s3.h
@@ -81,18 +81,11 @@ public:
 
 class LCRule_S3 : public LCRule
 {
-private:
-  CephContext *cct;
 public:
-  LCRule_S3(): cct(nullptr) {}
-  explicit LCRule_S3(CephContext *_cct): cct(_cct) {}
+  LCRule_S3() {}
 
   void dump_xml(Formatter *f) const;
   void decode_xml(XMLObj *obj);
-
-  void set_ctx(CephContext *ctx) {
-    cct = ctx;
-  }
 };
 
 class RGWLifecycleConfiguration_S3 : public RGWLifecycleConfiguration

diff --git a/src/rgw/rgw_op.cc b/src/rgw/rgw_op.cc
index 71caf89b40883742e202d722f0ce2b25e8a88af2..fc97e30df1d29b1597a202898b8605cd7846a505 100644 (file)
--- a/src/rgw/rgw_op.cc
+++ b/src/rgw/rgw_op.cc
@@ -5118,7 +5118,7 @@ void RGWPutLC::execute()
 {
   bufferlist bl;
   
-  RGWLifecycleConfiguration_S3 config;
+  RGWLifecycleConfiguration_S3 config(s->cct);
   RGWXMLParser parser;
   RGWLifecycleConfiguration_S3 new_config(s->cct);
 
@@ -5177,7 +5177,7 @@ void RGWPutLC::execute()
     RGWXMLDecoder::decode_xml("LifecycleConfiguration", config, &parser);
   } catch (RGWXMLDecoder::err& err) {
     ldpp_dout(this, 5) << "Bad lifecycle configuration: " << err << dendl;
-    op_ret = -EINVAL;
+    op_ret = -ERR_MALFORMED_XML;
     return;
   }
 

diff --git a/src/rgw/rgw_rest_s3.cc b/src/rgw/rgw_rest_s3.cc
index c65175788db8bf6fea4189cf525f342f2631f030..543a801857b3f7d9e49d3b2767fd7d26addf334c 100644 (file)
--- a/src/rgw/rgw_rest_s3.cc
+++ b/src/rgw/rgw_rest_s3.cc
@@ -2338,7 +2338,7 @@ void RGWGetLC_ObjStore_S3::send_response()
   if (op_ret < 0)
     return;
 
-  config.dump_xml(s->formatter);
+  encode_xml("LifecycleConfiguration", XMLNS_AWS_S3, config, s->formatter);
   rgw_flush_formatter_and_reset(s, s->formatter);
 }
 
@@ -2531,7 +2531,7 @@ public:
     if (!field)
       return 0;
 
-    string& s = field->get_data();
+    auto& s = field->get_data();
 
     if (stringcasecmp(s, "Requester") == 0) {
       *requester_pays = true;

diff --git a/src/rgw/rgw_rest_s3.h b/src/rgw/rgw_rest_s3.h
index c425f97c7f3c01b3d33beacfbfd400713233c021..3fb60f87550d63902bddd629648dcd92a3e2ba1d 100644 (file)
--- a/src/rgw/rgw_rest_s3.h
+++ b/src/rgw/rgw_rest_s3.h
@@ -298,7 +298,7 @@ public:
 
 class RGWGetLC_ObjStore_S3 : public RGWGetLC_ObjStore {
 protected:
-  RGWLifecycleConfiguration_S3  config;
+  RGWLifecycleConfiguration_S3 config;
 public:
   RGWGetLC_ObjStore_S3() {}
   ~RGWGetLC_ObjStore_S3() override {}

diff --git a/src/rgw/rgw_xml.cc b/src/rgw/rgw_xml.cc
index cc85286355f9776fab0fba284642203d83ccd5dd..9900c7bdea5a35f26d4f9af61b8be8cec630ffe9 100755 (executable)
--- a/src/rgw/rgw_xml.cc
+++ b/src/rgw/rgw_xml.cc
@@ -44,6 +44,16 @@ get_next()
   return obj;
 }
 
+bool XMLObjIter::get_name(string *name) const
+{
+  if (cur == end) {
+    return false;
+  }
+
+  *name = cur->first;
+  return true;
+}
+
 ostream& operator<<(ostream &out, const XMLObj &obj) {
    out << obj.obj_type << ": " << obj.data;
    return out;
@@ -77,12 +87,18 @@ xml_handle_data(const char *s, int len)
   data.append(s, len);
 }
 
-string& XMLObj::
+const string& XMLObj::
 XMLObj::get_data()
 {
   return data;
 }
 
+const string& XMLObj::
+XMLObj::get_obj_type()
+{
+  return obj_type;
+}
+
 XMLObj *XMLObj::
 XMLObj::get_parent()
 {
@@ -126,10 +142,7 @@ XMLObjIter XMLObj::find_first()
   map<string, XMLObj *>::iterator first;
   map<string, XMLObj *>::iterator last;
   first = children.begin();
-  if (first != children.end()) {
-    last = children.upper_bound(first->first);
-  }else
-    last = children.end();
+  last = children.end();
   iter.set(first, last);
   return iter;
 }
@@ -259,7 +272,7 @@ bool RGWXMLParser::parse(const char *_buf, int len, int done)
 
 void decode_xml_obj(unsigned long& val, XMLObj *obj)
 {
-  string& s = obj->get_data();
+  auto& s = obj->get_data();
   const char *start = s.c_str();
   char *p;
 

diff --git a/src/rgw/rgw_xml.h b/src/rgw/rgw_xml.h
index 38b2c702ecfd76d7e5a1f44e7d0bfff970f942bf..f86bdfae199a3bd7e05e6ffb9ccbe9b529eda708 100644 (file)
--- a/src/rgw/rgw_xml.h
+++ b/src/rgw/rgw_xml.h
@@ -21,6 +21,7 @@ public:
   ~XMLObjIter();
   void set(const XMLObjIter::map_iter_t &_cur, const XMLObjIter::map_iter_t &_end);
   XMLObj *get_next();
+  bool get_name(string *name) const;
 };
 
 /**
@@ -46,7 +47,8 @@ public:
   bool xml_start(XMLObj *parent, const char *el, const char **attr);
   virtual bool xml_end(const char *el);
   virtual void xml_handle_data(const char *s, int len);
-  string& get_data();
+  const string& get_data();
+  const string& get_obj_type();
   XMLObj *get_parent();
   void add_child(string el, XMLObj *obj);
   bool get_attr(string name, string& attr);
@@ -111,6 +113,9 @@ public:
   template<class T>
   static bool decode_xml(const char *name, T& val, XMLObj *obj, bool mandatory = false);
 
+  template<class T>
+  static bool decode_xml(const char *name, std::vector<T>& v, XMLObj *obj, bool mandatory = false);
+
   template<class C>
   static bool decode_xml(const char *name, C& container, void (*cb)(C&, XMLObj *obj), XMLObj *obj, bool mandatory = false);
 
@@ -160,21 +165,6 @@ void do_decode_xml_obj(list<T>& l, const string& name, XMLObj *obj)
   }
 }
 
-template<class T>
-void decode_xml_obj(std::vector<T>& v, XMLObj *obj)
-{
-  v.clear();
-
-  XMLObjIter iter = obj->find_first();
-  XMLObj *o;
-
-  while ((o = iter.get_next())) {
-    T val;
-    decode_xml_obj(val, o);
-    v.push_back(val);
-  }
-}
-
 template<class T>
 bool RGWXMLDecoder::decode_xml(const char *name, T& val, XMLObj *obj, bool mandatory)
 {
@@ -200,6 +190,36 @@ bool RGWXMLDecoder::decode_xml(const char *name, T& val, XMLObj *obj, bool manda
   return true;
 }
 
+template<class T>
+bool RGWXMLDecoder::decode_xml(const char *name, std::vector<T>& v, XMLObj *obj, bool mandatory)
+{
+  XMLObjIter iter = obj->find(name);
+  XMLObj *o = iter.get_next();
+
+  v.clear();
+
+  if (!o) {
+    if (mandatory) {
+      string s = "missing mandatory field " + string(name);
+      throw err(s);
+    }
+    return false;
+  }
+
+  do {
+    T val;
+    try {
+      decode_xml_obj(val, o);
+    } catch (err& e) {
+      string s = string(name) + ": ";
+      s.append(e.message);
+      throw err(s);
+    }
+    v.push_back(val);
+  } while ((o = iter.get_next()));
+  return true;
+}
+
 template<class C>
 bool RGWXMLDecoder::decode_xml(const char *name, C& container, void (*cb)(C&, XMLObj *), XMLObj *obj, bool mandatory)
 {
@@ -254,6 +274,14 @@ static void encode_xml(const char *name, const T& val, ceph::Formatter *f)
   f->close_section();
 }
 
+template<class T>
+static void encode_xml(const char *name, const char *ns, const T& val, ceph::Formatter *f)
+{
+  f->open_object_section_in_ns(name, ns);
+  val.dump_xml(f);
+  f->close_section();
+}
+
 void encode_xml(const char *name, const string& val, ceph::Formatter *f);
 void encode_xml(const char *name, const char *val, ceph::Formatter *f);
 void encode_xml(const char *name, bool val, ceph::Formatter *f);