rgw_cksum: address review comments

* remove rgw_cksum_pipe state enum, not needed [Casey review]
* remove a format that just took a single string substitution
  and passed it to an iostream [Casey review]
* use boost::to_upper* [Casey review]
* remove unused RGW_ATTR_CKSUM_ALGORITHM decl [Casey review]
* negate error code values in two places [Casey review]
* split cksum digests from base type decls
* resolve comment when checksum requested but not available
* remove redundant memset
* remove junk from rgw_blake3_digest.h
* s/ldpp_dout + fmt::format/ldpp_dout_fmt/g;
* fix conditional return of parts_count
      from RGWRados::Object::prepare().  A value for parts_count should
      be returned iff a *multipart* object manifest exists.
* remove /tmp output test
* finish moving ceph_crypto headers out of rgw_cksum.h
* consume the optional in multipart_parts_count
* target_attrs can be a reference (but not const)

Signed-off-by: Matt Benjamin <[email protected]>

Author: mattbenjamin

src/rgw/driver/rados/rgw_rados.cc

@@ -6750,7 +6750,10 @@ int RGWRados::Object::Read::prepare(optional_yield y, const DoutPrefixProvider *
   if (manifest /* params.parts_count */) {
       RGWObjManifest::obj_iterator end = manifest->obj_end(dpp);
       auto cur_part_id = end.get_cur_part_id();
-      params.parts_count = (cur_part_id == 1) ? 1 : cur_part_id - 1;;
+      if (cur_part_id != 0 ) {
+	/* end.get_cur_part_id() returns 0 for non-multipart manifests */
+	params.parts_count = (cur_part_id == 1) ? 1 : cur_part_id - 1;
+      }
   }
 
   if (!astate->exists) {

src/rgw/rgw_blake3_digest.h

@@ -19,9 +19,6 @@
 #include <stdio.h>
 #include "BLAKE3/c/blake3.h"
 
-#define XXH_INLINE_ALL 1 /* required for streaming variants */
-#include "xxhash.h"
-
 namespace rgw { namespace digest {
 
 class Blake3 {

src/rgw/rgw_cksum.h

@@ -13,6 +13,7 @@
 
 #pragma once
 
+#include <boost/algorithm/string/case_conv.hpp>
 #include <boost/algorithm/string/predicate.hpp>
 #include <cstdint>
 #include <cstring>
@@ -22,15 +23,9 @@
 #include <string_view>
 #include <array>
 #include <iterator>
-#include <boost/variant.hpp>
-#include <boost/blank.hpp>
 #include <boost/algorithm/string.hpp>
 #include "fmt/format.h"
-#include "common/ceph_crypto.h"
 #include "common/armor.h"
-#include "rgw_blake3_digest.h"
-#include "rgw_crc_digest.h"
-#include "rgw_xxh_digest.h"
 #include <boost/algorithm/hex.hpp>
 #include "rgw_hex.h"
 #include "rgw_b64.h"
@@ -83,11 +78,6 @@ namespace rgw { namespace cksum {
 
   namespace  ba = boost::algorithm;
 
-  static inline std::string safe_upcase_str(std::string s) {
-    std::transform(s.begin(), s.end(), s.begin(), ::toupper);
-    return s;
-  }
-
   class Cksum {
   public:
     static constexpr std::array<Desc, 8> checksums =
@@ -112,8 +102,10 @@ namespace rgw { namespace cksum {
     Cksum(Type _type, const char* _armored_text)
       : type(_type) {
       const auto& ckd = checksums[uint16_t(type)];
-      (void) ceph_unarmor((char*) digest.begin(), (char*) digest.begin() + ckd.digest_size,
-			  _armored_text, _armored_text + std::strlen(_armored_text));
+      (void) ceph_unarmor((char*) digest.begin(),
+			  (char*) digest.begin() + ckd.digest_size,
+			  _armored_text,
+			  _armored_text + std::strlen(_armored_text));
     }
 
     const char* type_string() const {
@@ -138,7 +130,7 @@ namespace rgw { namespace cksum {
 
     std::string element_name() const {
       std::string ts{type_string()};
-      return fmt::format("Checksum{}", safe_upcase_str(ts));
+      return fmt::format("Checksum{}", boost::to_upper_copy(ts));
     }
 
     std::string_view raw() const {
@@ -150,7 +142,6 @@ namespace rgw { namespace cksum {
       std::string hs;
       const auto& ckd = checksums[uint16_t(type)];
       hs.resize(ckd.armored_size);
-      memset(hs.data(), 0, hs.length());
       ceph_armor((char*) hs.data(), (char*) hs.data() + ckd.armored_size,
 		 (char*) digest.begin(), (char*) digest.begin() +
 		 ckd.digest_size);
@@ -199,6 +190,12 @@ namespace rgw { namespace cksum {
 
   static inline const std::optional<rgw::cksum::Cksum> no_cksum{std::nullopt};
 
+  static inline std::string to_string(const Type type) {
+    std::string hs;
+    const auto& ckd = Cksum::checksums[uint16_t(type)];
+    return ckd.name;
+  }
+
   static inline Type parse_cksum_type(const char* name)
   {
     for (const auto& ck : Cksum::checksums) {
@@ -226,117 +223,4 @@ namespace rgw { namespace cksum {
       parse_cksum_type_hdr(hdr_name) != Type::none;
   } /* is_cksum_hdr */
 
-  class Digest {
-  public:
-    virtual void Restart() = 0;
-    virtual void Update (const unsigned char *input, size_t length) = 0;
-    virtual void Update(const ceph::buffer::list& bl) = 0;
-    virtual void Final (unsigned char *digest) = 0;
-    virtual ~Digest() {}
-  };
-
-  template<class T>
-  class TDigest : public Digest
-  {
-    T d;
-  public:
-    TDigest() {}
-    TDigest(TDigest&& rhs) noexcept
-      : d(std::move(rhs.d))
-    {}
-    void Restart() override { d.Restart(); }
-    void Update(const unsigned char* data, uint64_t len) override {
-      d.Update(data, len);
-    }
-    void Update(const ceph::buffer::list& bl) {
-      for (auto& p : bl.buffers()) {
-	d.Update((const unsigned char *)p.c_str(), p.length());
-      }
-    }
-    void Final(unsigned char* digest) override {
-      d.Final(digest);
-    }
-  };
-
-  typedef TDigest<rgw::digest::Blake3> Blake3;
-  typedef TDigest<rgw::digest::Crc32> Crc32;
-  typedef TDigest<rgw::digest::Crc32c> Crc32c;
-  typedef TDigest<rgw::digest::XXH3> XXH3;
-  typedef TDigest<ceph::crypto::SHA1> SHA1;
-  typedef TDigest<ceph::crypto::SHA256> SHA256;
-  typedef TDigest<ceph::crypto::SHA512> SHA512;
-
-  typedef boost::variant<boost::blank,
-			 Blake3,
-			 Crc32,
-			 Crc32c,
-			 XXH3,
-			 SHA1,
-			 SHA256,
-			 SHA512> DigestVariant;
-
-  struct get_digest_ptr : public boost::static_visitor<Digest*>
-  {
-    get_digest_ptr() {};
-    Digest* operator()(const boost::blank& b) const { return nullptr; }
-    Digest* operator()(Blake3& digest) const { return &digest; }
-    Digest* operator()(Crc32& digest) const { return &digest; }
-    Digest* operator()(Crc32c& digest) const { return &digest; }
-    Digest* operator()(XXH3& digest) const { return &digest; }
-    Digest* operator()(SHA1& digest) const { return &digest; }
-    Digest* operator()(SHA256& digest) const { return &digest; }
-    Digest* operator()(SHA512& digest) const { return &digest; }
-  };
-
-  static inline Digest* get_digest(DigestVariant& ev)
-  {
-    return boost::apply_visitor(get_digest_ptr{}, ev);
-  }
-
-  static inline DigestVariant digest_factory(const Type cksum_type)
-  {
-    switch (cksum_type) {
-    case Type::blake3:
-      return Blake3();
-      break;
-    case Type::sha256:
-      return SHA256();
-      break;
-    case Type::crc32:
-      return Crc32();
-      break;
-    case Type::crc32c:
-      return Crc32c();
-      break;
-    case Type::xxh3:
-      return XXH3();
-      break;
-    case Type::sha512:
-      return SHA512();
-      break;
-    case Type::sha1:
-      return SHA1();
-      break;
-    case Type::none:
-      break;
-    };
-    return boost::blank();
-  } /* digest_factory */
-
-  static inline Cksum finalize_digest(Digest* digest, Type type)
-  {
-    Cksum cksum(type);
-    if (digest) {
-      auto data = cksum.digest.data();
-      digest->Final(data);
-    }
-    return cksum;
-  }
-
-  static inline std::string to_string(const Type type) {
-    std::string hs;
-    const auto& ckd = Cksum::checksums[uint16_t(type)];
-    return ckd.name;
-  }
-
 }} /* namespace */

src/rgw/rgw_cksum_digest.h

@@ -15,6 +15,7 @@
 
 #include <boost/variant.hpp>
 #include <boost/blank.hpp>
+#include "common/ceph_crypto.h"
 #include "rgw_blake3_digest.h"
 #include "rgw_crc_digest.h"
 #include "rgw_xxh_digest.h"

src/rgw/rgw_cksum_pipe.cc

@@ -30,8 +30,7 @@ namespace rgw::putobj {
     : Pipe(next),
       _type(_typ),
       dv(rgw::cksum::digest_factory(_type)),
-      _digest(cksum::get_digest(dv)), cksum_hdr(_hdr),
-      _state(State::DIGEST)
+      _digest(cksum::get_digest(dv)), cksum_hdr(_hdr)
   {}
 
   std::unique_ptr<RGWPutObj_Cksum> RGWPutObj_Cksum::Factory(

src/rgw/rgw_cksum_pipe.h

@@ -19,7 +19,8 @@
 #include <utility>
 #include <tuple>
 #include <cstring>
-#include "rgw_cksum.h"
+#include <boost/algorithm/string/case_conv.hpp>
+#include "rgw_cksum_digest.h"
 #include "rgw_common.h"
 #include "rgw_putobj.h"
 
@@ -76,7 +77,7 @@ namespace rgw::putobj {
 	 ix <= uint16_t(cksum::Type::blake3); ++ix) {
       cksum_type = cksum::Type(ix);
       auto hk = fmt::format("HTTP_X_AMZ_CHECKSUM_{}",
-			    safe_upcase_str(to_string(cksum_type)));
+			    boost::to_upper_copy(to_string(cksum_type)));
       auto hv = env.get(hk.c_str());
       if (hv) {
 	return
@@ -90,18 +91,11 @@ namespace rgw::putobj {
   // PutObj filter for streaming checksums
   class RGWPutObj_Cksum : public rgw::putobj::Pipe {
 
-    enum class State : uint16_t {
-      START,
-      DIGEST,
-      FINAL
-    };
-    
     cksum::Type _type;
     cksum::DigestVariant dv;
     cksum::Digest* _digest;
     cksum::Cksum _cksum;
     cksum_hdr_t cksum_hdr;
-    State _state;
 
   public:
 
@@ -118,15 +112,13 @@ namespace rgw::putobj {
     cksum::Type type() { return _type; }
     cksum::Digest* digest() const { return _digest; }
     const cksum::Cksum& cksum() { return _cksum; };
-    State state() const { return _state; }
 
     const cksum_hdr_t& header() const {
       return cksum_hdr;
     }
 
     const cksum::Cksum& finalize() {
       _cksum = finalize_digest(_digest, _type);
-      _state = State::FINAL;
       return _cksum;
     }
 
@@ -137,7 +129,7 @@ namespace rgw::putobj {
     }
 
     VerifyResult verify(const RGWEnv& env) {
-      if (_state == State::DIGEST) [[likely]] {
+      if (_cksum.type == cksum::Type::none) [[likely]] {
 	(void) finalize();
       }
       auto hv = expected(env);

src/rgw/rgw_common.h

@@ -26,6 +26,7 @@
 #include <boost/container/flat_map.hpp>
 #include <boost/container/flat_set.hpp>
 
+#include "common/dout_fmt.h"
 #include "common/ceph_crypto.h"
 #include "common/random_string.h"
 #include "common/tracer.h"
@@ -85,7 +86,6 @@ using ceph::crypto::MD5;
 #define RGW_ATTR_CORS		RGW_ATTR_PREFIX "cors"
 #define RGW_ATTR_ETAG RGW_ATTR_PREFIX "etag"
 #define RGW_ATTR_CKSUM    	RGW_ATTR_PREFIX "cksum"
-#define RGW_ATTR_CKSUM_ALGORITHM     RGW_ATTR_PREFIX "x-amz-checksum-algorithm"
 #define RGW_ATTR_BUCKETS	RGW_ATTR_PREFIX "buckets"
 #define RGW_ATTR_META_PREFIX	RGW_ATTR_PREFIX RGW_AMZ_META_PREFIX
 #define RGW_ATTR_CONTENT_TYPE	RGW_ATTR_PREFIX "content_type"
@@ -1992,16 +1992,6 @@ static inline std::string ys_header_mangle(std::string_view name)
   return out;
 } /* ys_header_mangle */
 
-static inline std::string& upcase_str(std::string& s) {
-  std::transform(s.begin(), s.end(), s.begin(), ::toupper);
-  return s;
-}
-
-static inline std::string safe_upcase_str(std::string s) {
-  std::transform(s.begin(), s.end(), s.begin(), ::toupper);
-  return s;
-}
-
 extern int rgw_bucket_parse_bucket_instance(const std::string& bucket_instance, std::string *bucket_name, std::string *bucket_id, int *shard_id);
 
 boost::intrusive_ptr<CephContext>