Implement QuicFileUtils by danzh2010 · Pull Request #6375 · envoyproxy/envoy

danzh2010 · 2019-03-25T19:10:43Z

Add QuicFileUtilsImpl using Envoy::FileSystem.

Risk Level: low
Testing: Added tests in test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc and tested with --define quiche=enabled
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-03-25T19:39:25Z

/assign @wu-bin

wu-bin · 2019-03-26T02:29:57Z

bazel/external/quiche.BUILD

    srcs = ["quiche/quic/platform/api/quic_mutex.cc"] + envoy_select_quiche(
-        ["quiche/quic/platform/api/quic_hostname_utils.cc"],
+        [
+            "quiche/quic/platform/api/quic_hostname_utils.cc",


Here and below, please keep lists in BUILD file sorted alphabetically. quic_file_utils should appear before quic_hostname_utils.

wu-bin · 2019-03-26T02:30:14Z

bazel/external/quiche.BUILD

    ] + envoy_select_quiche(
-        ["quiche/quic/platform/api/quic_hostname_utils.h"],
+        [
+            "quiche/quic/platform/api/quic_hostname_utils.h",


wu-bin · 2019-03-26T02:30:23Z

source/extensions/quic_listeners/quiche/platform/BUILD

    srcs = ["quic_cert_utils_impl.cc"] + envoy_select_quiche([
        "quic_hostname_utils_impl.cc",
        "quic_test_output_impl.cc",
+        "quic_file_utils_impl.cc",


wu-bin · 2019-03-26T02:42:50Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+
+namespace quic {
+
+void DFTraverseDirectory(const std::string& dirname, std::vector<std::string>& files) {


Please move this function to anonymous namespace, since it is only used in this file.

The second parameter, "files", should be a pointer instead of a mutable reference. (Envoy uses Google's C++ style guide for the most part)

Please move this function to anonymous namespace, since it is only used in this file.
done
The second parameter, "files", should be a pointer instead of a mutable reference. (Envoy uses Google's C++ style guide for the most part)
Envoy also prefers non-const reference if the pass-in argument is not supposed to be null.

wu-bin · 2019-03-26T02:53:19Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+      break;
+    case Envoy::Filesystem::FileType::Directory:
+      if (entry.name_ != "." && entry.name_ != "..") {
+        DFTraverseDirectory(absl::StrCat(dirname, "/", entry.name_), files);


Does it work if "dirname" already ends with '/'?

Yes, but the file name returned also contains double "/". The only place used in quic gives path with no-trailing '/'. I added comment that it takes path without trailing '/'

SG given how infrequent this is used. In general, I'd prefer ReadFileContentsImpl to remove the trailing '/' if needed.

wu-bin · 2019-03-26T02:54:53Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+      }
+      break;
+    default:
+      ASSERT(false, "Unknow file entry under directory " + dirname);


Include "entry.type_" in the error message?

wu-bin · 2019-03-26T03:08:17Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+      dir_path_ + "/file", dir_path_ + "/sub_dir1/sub_file1",
+      dir_path_ + "/sub_dir1/sub_dir1_1/sub_file1_1", dir_path_ + "/sub_dir2/sub_file2"};
+  std::vector<std::string> files = ReadFileContents(dir_path_);
+  EXPECT_EQ(expected.size(), files.size());


Line 505 to 508: You can use EXPECT_THAT(files, UnorderedElementsAre(expected))

good point!

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin

LGTM.

wu-bin · 2019-03-26T16:27:33Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+      break;
+    case Envoy::Filesystem::FileType::Directory:
+      if (entry.name_ != "." && entry.name_ != "..") {
+        DFTraverseDirectory(absl::StrCat(dirname, "/", entry.name_), files);


SG given how infrequent this is used. In general, I'd prefer ReadFileContentsImpl to remove the trailing '/' if needed.

danzh2010 · 2019-03-26T17:14:33Z

/assign @alyssawilk

alyssawilk

LGTM modulo one naming nit!

alyssawilk · 2019-03-26T18:43:15Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+namespace quic {
+namespace {
+
+void DFTraverseDirectory(const std::string& dirname, std::vector<std::string>& files) {


Naming? I'm not entirely sure what DF stands for here.

Rename to depthFirstTraverseDirectory

alyssawilk · 2019-03-26T18:46:09Z

source/extensions/quic_listeners/quiche/platform/quic_file_utils_impl.cc

+} // namespace
+
+// Traverses the directory |dirname| and returns all of the files it contains.
+std::vector<std::string> ReadFileContentsImpl(const std::string& dirname) {


Comment for "upstream", I think this is badly named as it's not reading the file contents.
I don't think you can fix that here though :-/

Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 3 commits March 25, 2019 15:02

Add QuicFileUtilsImpl

74df0e0

Signed-off-by: Dan Zhang <danzh@google.com>

add test

96022d4

Signed-off-by: Dan Zhang <danzh@google.com>

BUILD file

7227b93

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners March 25, 2019 19:10

repokitteh-read-only bot assigned wu-bin Mar 25, 2019

wu-bin suggested changes Mar 26, 2019

View reviewed changes

use UnorderedElementsAre

824c506

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin previously approved these changes Mar 26, 2019

View reviewed changes

repokitteh-read-only bot assigned alyssawilk Mar 26, 2019

alyssawilk reviewed Mar 26, 2019

View reviewed changes

danzh1989 added 2 commits March 26, 2019 16:47

rename

4d38efb

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filesystem

58c70c6

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wu-bin’s stale review via 58c70c6 March 26, 2019 20:51

alyssawilk approved these changes Mar 26, 2019

View reviewed changes

alyssawilk merged commit a450777 into envoyproxy:master Mar 27, 2019


		namespace quic {

		void DFTraverseDirectory(const std::string& dirname, std::vector<std::string>& files) {

Conversation

danzh2010 commented Mar 25, 2019

Uh oh!

danzh2010 commented Mar 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Mar 26, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants