Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
13 changes: 9 additions & 4 deletions bazel/envoy_build_system.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ def envoy_cc_test(
deps = [],
tags = [],
args = [],
copts = [],
shard_count = None,
coverage = True,
local = False,
Expand All @@ -457,10 +458,11 @@ def envoy_cc_test(
deps = deps,
repository = repository,
tags = test_lib_tags,
copts = copts,
)
native.cc_test(
name = name,
copts = envoy_copts(repository, test = True),
copts = envoy_copts(repository, test = True) + copts,
linkopts = envoy_test_linkopts(),
linkstatic = 1,
malloc = tcmalloc_external_dep(repository),
Expand Down Expand Up @@ -488,13 +490,14 @@ def envoy_cc_test_infrastructure_library(
deps = [],
repository = "",
tags = [],
include_prefix = None):
include_prefix = None,
copts = []):
native.cc_library(
name = name,
srcs = srcs,
hdrs = hdrs,
data = data,
copts = envoy_copts(repository, test = True),
copts = envoy_copts(repository, test = True) + copts,
testonly = 1,
deps = deps + [envoy_external_dep_path(dep) for dep in external_deps] + [
envoy_external_dep_path("googletest"),
Expand All @@ -517,7 +520,8 @@ def envoy_cc_test_library(
deps = [],
repository = "",
tags = [],
include_prefix = None):
include_prefix = None,
copts = []):
deps = deps + [
repository + "//test/test_common:printers_includes",
]
Expand All @@ -531,6 +535,7 @@ def envoy_cc_test_library(
repository,
tags,
include_prefix,
copts,
)

# Envoy test binaries should be specified with this function.
Expand Down
43 changes: 43 additions & 0 deletions bazel/external/quiche.BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ load(":genrule_cmd.bzl", "genrule_cmd")
load(
"@envoy//bazel:envoy_build_system.bzl",
"envoy_cc_test",
"envoy_copts",
"envoy_select_quiche",
)

Expand Down Expand Up @@ -253,6 +254,48 @@ cc_library(
deps = [":quic_platform_export"],
)

cc_library(
name = "epoll_server_platform",
testonly = 1,
hdrs = [
"quiche/epoll_server/platform/api/epoll_address_test_utils.h",
"quiche/epoll_server/platform/api/epoll_bug.h",
"quiche/epoll_server/platform/api/epoll_expect_bug.h",
"quiche/epoll_server/platform/api/epoll_export.h",
"quiche/epoll_server/platform/api/epoll_logging.h",
"quiche/epoll_server/platform/api/epoll_ptr_util.h",
"quiche/epoll_server/platform/api/epoll_test.h",
"quiche/epoll_server/platform/api/epoll_thread.h",
"quiche/epoll_server/platform/api/epoll_time.h",
],
visibility = ["//visibility:public"],
deps = ["@envoy//test/extensions/quic_listeners/quiche/platform:epoll_server_platform_impl_lib"],
)

cc_library(

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.

Why are these cc_library rather than envoy_cc_library? The latter is preferred, it simplifies Google import and removes boilerplate.

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.

I think the original intention was that these targets are not in the envoy workspace and to use envoy_cc_library we need to explicitly specify repository = "@envoy". But since it benefits to code import, I just switched to use envoy_cc_library.

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.

I guess internally we will use a different QUICHE binding actually. But, it's good either way.

name = "epoll_server_lib",
testonly = 1,
srcs = [
"quiche/epoll_server/fake_simple_epoll_server.cc",
"quiche/epoll_server/simple_epoll_server.cc",
],
hdrs = [
"quiche/epoll_server/fake_simple_epoll_server.h",
"quiche/epoll_server/simple_epoll_server.h",
],
copts = envoy_copts("@envoy") + ["-Wno-error=unused-parameter"],

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.

Instead of ignoring it, can we fix the unused-parameter warning in epoll_server code? We don't need to change envoy_build_system.bzl that way.

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.

QUICHE allows unused-parameter, so I think we will run into this warning in quic and h2 code as well. That's why I modify envoy_build_system.bzl. Suppressing this warning in Envoy seems more reasonable than adding this enforcement in QUICHE.

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.

SGTM, please move "-Wno-error=unused-parameter" into a global list and use it in here and epoll_server_test.

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.

In case you missed it, this comment still needs to be resolved. @danzh2010

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.

Where to put the global list? envoy_build_system.bzl?

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.

In this file(bazel/external/quiche.BUILD), since it's only needed by quiche build rules.

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.

done, and change it to -Wno-unused-parameter to suppress the warning entirely. Otherwise there will be too many warning output.

visibility = ["//visibility:public"],
deps = [":epoll_server_platform"],
)

envoy_cc_test(
name = "epoll_server_test",

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.

This test is also defined in test/extensions/quic_listeners/quiche/platform/BUILD, should we delete one of them?

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.

Sorry, the one is //test/ is used for debugging. I removed that one instead.

srcs = ["quiche/epoll_server/simple_epoll_server_test.cc"],
copts = ["-Wno-error=unused-parameter"],
repository = "@envoy",
deps = [":epoll_server_lib"],
)

envoy_cc_test(
name = "http2_platform_test",
srcs = envoy_select_quiche(
Expand Down
3 changes: 3 additions & 0 deletions bazel/external/quiche.genrule_cmd
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ cat <<EOF >sed_commands
# Rewrite include directives for platform impl files.
/^#include/ s!net/(http2|spdy|quic)/platform/impl/!extensions/quic_listeners/quiche/platform/!

# Rewrite include directives for epoll_server platform impl files.
/^#include/ s!net/tools/epoll_server/platform/impl!test/extensions/quic_listeners/quiche/platform/!

# Strip "net/third_party" from include directives to other QUICHE files.
/^#include/ s!net/third_party/quiche/src/!quiche/!

Expand Down
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,8 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/google/subpar/archive/1.3.0.tar.gz"],
),
com_googlesource_quiche = dict(
# Static snapshot of https://quiche.googlesource.com/quiche/+archive/840edb6d672931ff936004fc35a82ecac6060844.tar.gz
sha256 = "1aba26cec596e9f3b52d93fe40e1640c854e3a4c8949e362647f67eb8e2382e3",
urls = ["https://storage.googleapis.com/quiche-envoy-integration/840edb6d672931ff936004fc35a82ecac6060844.tar.gz"],
# Static snapshot of https://quiche.googlesource.com/quiche/+archive/3e188a56edbcc471799499958bfd7c05ec45b9e2.tar.gz
sha256 = "b116cfac5d39390c00f13758b21978aab953d8a67d846182f39b333644ee64b2",
urls = ["https://storage.googleapis.com/quiche-envoy-integration/3e188a56edbcc471799499958bfd7c05ec45b9e2.tar.gz"],
),
)
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,18 @@
#define CHECK(condition) \
QUIC_LOG_IF_IMPL(FATAL, ABSL_PREDICT_FALSE(!(condition))) << "CHECK failed: " #condition "."

#define CHECK_GT(a, b) CHECK((a) > (b))
#define CHECK_GE(a, b) CHECK((a) >= (b))
#define CHECK_LT(a, b) CHECK((a) < (b))
#define CHECK_LE(a, b) CHECK((a) <= (b))
#define CHECK_NE(a, b) CHECK((a) != (b))
#define CHECK_EQ(a, b) CHECK((a) == (b))

#ifdef NDEBUG
// Release build
#define DCHECK(condition) QUIC_COMPILED_OUT_LOG()
#define DCHECK(condition) \

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.

I'm curious, what is the problem without this change?

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.

if we have:
{
bool a = true;
DCHECK(a)
}

In release build, w/o this change a is not used. We can supress unused-variable too. But that seems to be over-kill as QUICHE also forbids unused-variable.

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.

Got it. There are other macros defined to "QUIC_COMPILED_OUT_LOG" as well, maybe change "QUIC_COMPILED_OUT_LOG" to take a "condition" instead?

#define QUIC_COMPILED_OUT_LOG(condition) QUIC_LOG_IMPL_INTERNAL(false && (condition), quic::NullLogStream().stream())

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.

done

while (false && (condition)) \
QUIC_COMPILED_OUT_LOG()
#define QUIC_COMPILED_OUT_LOG() QUIC_LOG_IMPL_INTERNAL(false, quic::NullLogStream().stream())
#define QUIC_DVLOG_IMPL(verbosity) QUIC_COMPILED_OUT_LOG()
#define QUIC_DVLOG_IF_IMPL(verbosity, condition) QUIC_COMPILED_OUT_LOG()
Expand All @@ -83,6 +92,11 @@
#endif

#define DCHECK_GE(a, b) DCHECK((a) >= (b))
#define DCHECK_GT(a, b) DCHECK((a) > (b))
#define DCHECK_LT(a, b) DCHECK((a) < (b))
#define DCHECK_LE(a, b) DCHECK((a) <= (b))
#define DCHECK_NE(a, b) DCHECK((a) != (b))
#define DCHECK_EQ(a, b) DCHECK((a) == (b))

#define QUIC_PREDICT_FALSE_IMPL(x) ABSL_PREDICT_FALSE(x)

Expand Down
32 changes: 32 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,35 @@ envoy_cc_test_library(
"//test/test_common:environment_lib",
],
)

envoy_cc_test_library(
name = "epoll_server_platform_impl_lib",
hdrs = [
"epoll_address_test_utils_impl.h",
"epoll_bug_impl.h",
"epoll_expect_bug_impl.h",
"epoll_export_impl.h",
"epoll_logging_impl.h",
"epoll_ptr_util_impl.h",
"epoll_test_impl.h",
"epoll_thread_impl.h",
"epoll_time_impl.h",
],
external_deps = ["abseil_time"],
deps = [
"//include/envoy/network:address_interface",
"//source/extensions/quic_listeners/quiche/platform:quic_platform_base_impl_lib",
"//test/test_common:environment_lib",
],
)

envoy_cc_test(
name = "epoll_server_test",
srcs = [
"simple_epoll_server_test.cc",
],
copts = ["-Wno-error=unused-parameter"],
deps = [
"@com_googlesource_quiche//:epoll_server_lib",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include <sys/socket.h>

#include "envoy/network/address.h"

#include "test/test_common/environment.h"

namespace epoll_server {

namespace {

int addressFamilyUnderTestHelper() {
std::vector<Envoy::Network::Address::IpVersion> versions =
Envoy::TestEnvironment::getIpVersionsForTest();

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.

How about using std::find for clarity?

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v4) {
return AF_INET;
}

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v6) {
return AF_INET6;
}

return -1;

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.

done

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.

hmm, my code in comment doesn't actually work, it should be

if (std::find(versions.begin(), versions.end(), Envoy::Network::Address::IpVersion::v4) != versions.end()) {
return AF_INET;
}
// Same for v6

if (versions.size() == 2 ||
(versions.size() == 1 && versions[0] == Envoy::Network::Address::IpVersion::v4)) {
return AF_INET;
} else if (versions.size() == 1) {
ASSERT(versions[0] == Envoy::Network::Address::IpVersion::v6);
return AF_INET6;
}
return -1;
}

} // namespace

int AddressFamilyUnderTestImpl() {

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.

Looks like this is confusingly named, but doing the right thing.
Possibly worth a rename or comments that this returns the "highest" supported address version of v4/v6. Given it's test code and I suspect will be unused I'm fine landing as-is.

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.

Renaming requires upstream change. I added a comment here for the return value.

static const int* version = new int(addressFamilyUnderTestHelper());

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.

nit: no need to call new since int is trivial to destruct, just do:

static const int version = addressFamilyUnderTestHelper();
return version;

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.

done

return *version;
}

} // namespace epoll_server
11 changes: 11 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/epoll_bug_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h"

#define EPOLL_BUG_IMPL QUIC_BUG_IMPL
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_expect_bug_impl.h"

#define EXPECT_EPOLL_BUG_IMPL EXPECT_QUIC_BUG_IMPL
10 changes: 10 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/epoll_export_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#define EPOLL_EXPORT
#define EPOLL_EXPORT_PRIVATE
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_logging_impl.h"

namespace epoll_server {

#define EPOLL_LOG_IMPL(severity) QUIC_LOG_IMPL(severity)
#define EPOLL_VLOG_IMPL(verbosity) QUIC_VLOG_IMPL(verbosity)

#define EPOLL_PLOG_IMPL(severity) QUIC_PLOG_IMPL(severity)

#ifndef NDEBUG
#define EPOLL_DVLOG_IMPL(verbosity) QUIC_VLOG_IMPL(verbosity)

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.

Why not
#define EPOLL_DVLOG_IMPL(verbosity) QUIC_DVLOG_IMPL(verbosity)
?

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.

good point. done!

#else
#define EPOLL_DVLOG_IMPL(verbosity) QUIC_VLOG_IF_IMPL(verbosity, false)
#endif

} // namespace epoll_server
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include <memory>

namespace epoll_server {

template <typename T, typename... Args> std::unique_ptr<T> EpollMakeUniqueImpl(Args&&... args) {

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.

nit:

template <typename T, typename... Args>
using EpollMakeUniqueImpl = std::make_unique<T, Args...>;

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.

alias doesn't work with function template.

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.

Sorry about the wrong suggestion, and thanks for trying:)

return std::make_unique<T>(std::forward<Args>(args)...);
}

} // namespace epoll_server
12 changes: 12 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/epoll_test_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "gmock/gmock.h"
#include "gtest/gtest.h"

#define EpollTestImpl ::testing::Test

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.

nit: prefer type alias to macro:

namespace epoll_server {
using EpollTestImpl = ::testing::Test;
} // namespace epoll_server

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.

done

15 changes: 15 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/epoll_thread_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_thread_impl.h"

namespace epoll_server {

using EpollThreadImpl = quic::QuicThreadImpl;

} // namespace epoll_server
15 changes: 15 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/epoll_time_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#pragma once

// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "absl/time/clock.h"

namespace epoll_server {

inline int64_t WallTimeNowInUsecImpl() { return absl::GetCurrentTimeNanos() / 1000; }

} // namespace epoll_server