New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: add variadic string join for s3 signature generation #15678

Merged
merged 5 commits into from Jun 16, 2017

Conversation

Projects
None yet
3 participants
@cbodley
Contributor

cbodley commented Jun 14, 2017

No description provided.

@cbodley cbodley added the rgw label Jun 14, 2017

@cbodley cbodley requested review from adamemerson and rzarzynski Jun 14, 2017

// specializations for null-terminated string literals
template <std::size_t N>
struct string_traits<const char[N]> {
static constexpr size_t size(const char (&s)[N]) { return N-1; }

This comment has been minimized.

@rzarzynski

rzarzynski Jun 14, 2017

Contributor

This gives different results than std::strlen for strings that have \0 in the middle. However, I think we can live with that for now.

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

yeah, i think i prefer strlen() here too. that's what the boost::string_view() constructor does for string literals, so this would make string_size() more consistent with those conversions in append/join

This comment has been minimized.

@rzarzynski

rzarzynski Jun 14, 2017

Contributor

We share absolutely the same feeling. The issue is that astrlen implementation isn't mandated to be conestexpr-capable. Clang was complaining about that today. :-(

struct ltstr_nocase
{
bool operator()(const string& s1, const string& s2) const
bool operator()(const std::string& s1, const std::string& s2) const

This comment has been minimized.

@rzarzynski

rzarzynski Jun 14, 2017

Contributor

Thanks!

.append(AWS4_EMPTY_PAYLOAD_HASH, empty_hash_len)
.append("\n", std::strlen("\n"))
.append(payload_hash);
const auto string_to_sign = string_join_reserve("\n",

This comment has been minimized.

@rzarzynski

rzarzynski Jun 14, 2017

Contributor

This looks really nice!

This comment has been minimized.

@adamemerson

adamemerson Jun 14, 2017

Contributor

Faster AND cleaner.

This comment has been minimized.

@cbodley

cbodley Jun 14, 2017

Contributor

i'm not sure it's actually faster, as it was already calculating the total size for string::reserve(). i just think the variadic version is less error-prone by doing the size calculation for you

const size_t total_len = algorithm_len + date.length() + \
credential_scope.length() + prev_chunk_signature.length() + \
empty_hash_len + payload_hash.length() + std::strlen("\n") * 5;
const auto string_to_sign = create_n_reserve<std::string>(total_len)

This comment has been minimized.

@rzarzynski

rzarzynski Jun 14, 2017

Contributor

I think we can safely drop create_n_reserve from rgw_string. It doesn't seem to be used anywhere else.

rgw: rgw_string.h qualifies string with std::
this allows it to be included as a standalone header, without relying on
other headers to pull in namespace std

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@adamemerson

Libertine Gazelles Try Multiplication

.append(AWS4_EMPTY_PAYLOAD_HASH, empty_hash_len)
.append("\n", std::strlen("\n"))
.append(payload_hash);
const auto string_to_sign = string_join_reserve("\n",

This comment has been minimized.

@adamemerson

adamemerson Jun 14, 2017

Contributor

Faster AND cleaner.

cbodley added some commits Jun 14, 2017

rgw: add variadic string join and cat
these variadic functions are able to calculate the total length of the
given string arguments for std::string::reserve(). string arguments can
be c strings, string literals, or any string class that provides a
.size(), .begin(), and .end()

Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: add unit tests for variadic string join/cat
Signed-off-by: Casey Bodley <cbodley@redhat.com>
rgw: use string_join_reserve() for signature generation
Signed-off-by: Casey Bodley <cbodley@redhat.com>
rgw: remove unused create_n_reserve()
Signed-off-by: Casey Bodley <cbodley@redhat.com>

@cbodley cbodley changed the title from [RFC] rgw: add variadic string join for s3 signature generation to rgw: add variadic string join for s3 signature generation Jun 14, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 14, 2017

rebased over constexpr changes, updated with strlen() for the array specializations, and removed create_n_reserve()

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 14, 2017

jenkins test this please

1 similar comment
@cbodley

This comment has been minimized.

Contributor

cbodley commented Jun 14, 2017

jenkins test this please

@cbodley cbodley merged commit 4c22420 into ceph:master Jun 16, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
arm64 make check arm64 make check succeeded
Details
make check make check succeeded
Details

@cbodley cbodley deleted the cbodley:wip-rgw-string-join branch Jun 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment