Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions include/envoy/http/header_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ class HeaderString {
*/
const char* c_str() const { return buffer_.ref_; }

/**
* @return a std::string.
*/
std::string getString() const {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? Seems pretty superfluous to me, but I don't feel that strongly about it. If it is, I suspect you don't need the if statement and the constructor will do the right thing if you pass a length of 0, but I'm not sure.

@saumoh saumoh Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the need for it came up in the feedback for pr #2415
it is kind of useful to have it here and not have the user have to convert a c_str(). thx

if (string_length_) {
return std::string(buffer_.ref_, string_length_);
}
return "";
}

/**
* Return the string to a default state. Reference strings are not touched. Both inline/dynamic
* strings are reset to zero size.
Expand Down
15 changes: 15 additions & 0 deletions include/envoy/http/protocol.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#pragma once
#include <string>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this include be removed?


namespace Envoy {
namespace Http {
Expand All @@ -10,5 +11,19 @@ namespace Http {
enum class Protocol { Http10, Http11, Http2 };
const size_t NumProtocols = 3;

inline std::string getProtocolString(const Protocol& p) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already implemented here: https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/access_log_formatter.cc#L33

Please consolidate (and use the string in access log formatter to be consistent).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consolidated. Thanks.

switch (p) {
case Protocol::Http10:
return std::string("Http1.0");
case Protocol::Http11:
return std::string("Http1.1");
case Protocol::Http2:
return std::string("Http2");
default:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mild preference for no default - I'd rather have a compile fail if we add a new Protocol and forget to update this function.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 just return directly and avoid default. Add NOT_REACHED at end of function if necessary.

break;
}
return "";
}

} // namespace Http
} // namespace Envoy
1 change: 1 addition & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"//source/common/protobuf",
"@envoy_api//envoy/api/v2:address_cc",
"@envoy_api//envoy/api/v2:base_cc",
],
)
12 changes: 12 additions & 0 deletions source/common/network/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -371,5 +371,17 @@ absl::uint128 Utility::flipOrder(const absl::uint128& input) {
return result;
}

void Utility::addressToProtobufAddress(const Address::Instance& address,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already implemented here: https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L67 please consolidate

Also, a dedicated function like this needs dedicated tests.

@saumoh saumoh Feb 1, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests and consolidated. thanks.

envoy::api::v2::Address& proto_address) {
if (address.type() == Address::Type::Pipe) {
proto_address.mutable_pipe()->set_path(address.asString());
} else {
ASSERT(address.type() == Address::Type::Ip);
auto* socket_address = proto_address.mutable_socket_address();
socket_address->set_address(address.ip()->addressAsString());
socket_address->set_port_value(address.ip()->port());
}
}

} // namespace Network
} // namespace Envoy
8 changes: 8 additions & 0 deletions source/common/network/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,14 @@ class Utility {
*/
static absl::uint128 Ip6htonl(const absl::uint128& address);

/**
* Copies the address instance into the protobuf representation of an address.
* @param address is the address to be copied into the protobuf representation of this address.
* @param proto_address is the protobuf address to which the address instance is copied into.
*/
static void addressToProtobufAddress(const Address::Instance& address,
envoy::api::v2::Address& proto_address);

private:
static void throwWithMalformedIp(const std::string& ip_address);

Expand Down
9 changes: 9 additions & 0 deletions test/common/http/header_map_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,15 @@ TEST(HeaderStringTest, All) {
EXPECT_FALSE(string.caseInsensitiveContains("keep-alive"));
EXPECT_FALSE(string.caseInsensitiveContains(""));
}

// getString
{
std::string static_string("HELLO");
HeaderString headerString(static_string);
std::string retString = headerString.getString();
EXPECT_EQ("HELLO", retString);
EXPECT_EQ(5U, retString.size());
}
}

TEST(HeaderMapImplTest, InlineInsert) {
Expand Down