Implements cachetool to manipulate simple typed cache.

net/tools/cachetool/cachetool.cc

Index: net/tools/cachetool/cachetool.cc
diff --git a/net/tools/cachetool/cachetool.cc b/net/tools/cachetool/cachetool.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9fb2094d0fbc35e0e04c6c8eaac72e218fa474ab
--- /dev/null
+++ b/net/tools/cachetool/cachetool.cc
@@ -0,0 +1,187 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <iostream>
+
+#include "base/at_exit.h"
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "net/base/io_buffer.h"
+#include "net/base/test_completion_callback.h"
+#include "net/disk_cache/disk_cache.h"
+#include "net/disk_cache/simple/simple_backend_impl.h"
+#include "net/http/http_cache.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+
+using namespace disk_cache;

[gavinp, 2016/02/19 16:21:11]

See https://google.github.io/styleguide/cppguide.html#Namespaces , which  says,
in part:

      "You may not use a using-directive to make all names from a namespace
available."

Enumerate the names you'd like to use, or qualify each use. Sorry.

[gabadie, 2016/02/22 14:35:46]

Done.

+
+namespace {
+
+base::MessageLoopForIO* g_message_loop = nullptr;
+
+// Create a simple typed cache back-end.

[gavinp, 2016/02/19 16:21:11]

grammar: "Create a SimpleCache backend."

But at that point, it recapitulates the function name, so might as well delete
the comment.

[gabadie, 2016/02/22 14:35:45]

Done.

+Backend* CreateSimpleBackend(const base::FilePath& cache_path) {

[gavinp, 2016/02/19 16:21:11]

This should return scoped_ptr<Backend> or scoped_ptr<SimpleBackendImpl>. I
suggest the former.

Bonus points: figure out if the code Egor wrote in
net/disk_cache/cache_creator.cc is worth using? I think not, Egor probably would
agree. If he disagrees, he's probably the one who's right.

[gabadie, 2016/02/22 14:35:46]

Egor not here yet. We can universalize this tool latter on.

+  scoped_ptr<SimpleBackendImpl> cache_backend(new SimpleBackendImpl(
+      cache_path, 0, net::DISK_CACHE, g_message_loop->task_runner(), NULL));
+
+  net::TestCompletionCallback cb;
+  int rv = cache_backend->Init(cb.callback());
+  DCHECK_EQ(net::OK, cb.GetResult(rv));

[gavinp, 2016/02/19 16:21:11]

This somewhat defeats the purpose of your later check on the return value of
this method. Wouldn't:

if (cb.GetResult(rv) != net::OK)
  return nullptr;

be more in keeping with the apparent goal of returning with a useful error
message when a non-simple-cache directory is given?

[gabadie, 2016/02/22 14:35:46]

Oups... Done.

+
+  return cache_backend.release();
+}
+
+// Prints all cache's keys to the stdout.

[gavinp, 2016/02/19 16:21:12]

"Print call of a cache's keys to stdout."

Explanation: "cache" requires an indefinite article here in English.

As well, stdout does not require, and is incorrect if you use a definite article
here. Compare "France is a European country" and "La France est un État de
l'Europe", which contains two good examples of definite articles disappearing in
translation to English. You may be happy to learn that I am constantly failing
to add articles when I attempt to write or speak la française.

[gabadie, 2016/02/22 14:35:46]

Ahah ^^ Thanks for that outstanding example! =)
Done.

+int ListKeys(Backend* cache_backend) {

[gavinp, 2016/02/19 16:21:11]

I think "bool" is a better return code for ListKeys and the other printers; we
aren't particularly propogating interesting information about our error codes
(this function has NO POSSIBLE ERRORS, the other printer only ever returns 0 or
1), and so I don't see a justification for inverting our sense of right and
wrong.

[gabadie, 2016/02/22 14:35:45]

Done.

+  scoped_ptr<Backend::Iterator> entry_iterator =
+      cache_backend->CreateIterator();
+  Entry* entry = nullptr;
+  net::TestCompletionCallback cb;
+  int rv = entry_iterator->OpenNextEntry(&entry, cb.callback());
+  while (cb.GetResult(rv) == net::OK) {
+    std::string url = entry->GetKey();
+    std::cout << url << std::endl;
+    entry->Close();
+    entry = nullptr;
+    rv = entry_iterator->OpenNextEntry(&entry, cb.callback());
+  }
+  return 0;
+}
+
+// Prints a key's stream to the stdout.

[gavinp, 2016/02/19 16:21:11]

"Print a key's stream to stdout."

[gabadie, 2016/02/22 14:35:46]

Done.

+int GetKeyStream(Backend* cache_backend, const std::string& key, int index) {

[gavinp, 2016/02/19 16:21:11]

I'm not a huge fan of this API; the magic index values of 0 and 1 are bad. Could
we make |index| an enum class argument perhaps? Frustrating that we'll be
recapitulating the constant from the HttpCache, but I think it improves
readability.

[gabadie, 2016/02/22 14:35:45]

I totally agree this issue makes the code not pretty.

But to me, adding enum class only in this non production code peace would not be
the proper thing to do. The correct fix would be a tiny refactor to change the
API to use the enum class instead. Sadly, this is not the point of this change.

This command line tool is more a offline plumbing work, than maintained code for
user experience. For more context, this tool is meant to be used for other
plumbing work (hack?) to make the NoState-Prefetch experimental benchmark
working. Therefore I would prefer to make this tool as thin as possible: not
adding any candy abstractions in mapping enums <-> index and doing other stuff
such as passing string to the command line instead of an integer.

[gavinp, 2016/02/22 15:44:56]

Fair point. And you're placing the blame where it belongs.

+  if (index < 0 || index > 2) {

[gavinp, 2016/02/19 16:21:11]

"2" is really problematic here. Why is that the maximum? An enumeration helps
here again.

[gabadie, 2016/02/22 14:35:46]

Stuffing with comment.

+    std::cerr << "Invalid stream index." << std::endl;
+    return 1;
+  }
+
+  Entry* cache_entry;
+  net::TestCompletionCallback cb;
+  int rv = cache_backend->OpenEntry(key, &cache_entry, cb.callback());
+  if (cb.GetResult(rv) != net::OK) {
+    std::cerr << "Couldn't find key." << std::endl;

[gavinp, 2016/02/19 16:21:12]

Isn't it the entry that we couldn't find?

[gabadie, 2016/02/22 14:35:46]

Done.

+    return 1;
+  }
+
+  const int kInitBufferSize = 8192;
+  scoped_refptr<net::GrowableIOBuffer> buffer(new net::GrowableIOBuffer());
+  buffer->SetCapacity(kInitBufferSize);
+  for (;;) {
+    rv = cache_entry->ReadData(index, buffer->offset(), buffer.get(),
+                               buffer->capacity() - buffer->offset(),
+                               cb.callback());
+    rv = cb.GetResult(rv);
+    if (rv < 0) {
+      cache_entry->Close();
+      std::cerr << "Stream read error." << std::endl;
+      return 1;
+    }
+    buffer->set_offset(buffer->offset() + rv);
+    if (rv == 0) {
+      break;
+    }
+    buffer->SetCapacity(buffer->offset() * 2);
+  }
+  cache_entry->Close();
+  if (index == 0) {
+    net::HttpResponseInfo response_info;
+    bool truncated_response_info = false;
+    net::HttpCache::ParseResponseInfo(buffer->StartOfBuffer(), buffer->offset(),
+                                      &response_info, &truncated_response_info);
+    if (truncated_response_info) {
+      std::cerr << "Truncated HTTP response." << std::endl;
+      return 1;
+    }
+    std::cout << net::HttpUtil::ConvertHeadersBackToHTTPResponse(
+        response_info.headers->raw_headers());
+  } else {
+    fwrite(buffer->StartOfBuffer(), 1, buffer->offset(), stdout);

[gavinp, 2016/02/19 16:21:11]

Intermixing fwrite and std::cout is dangerous. I believe this use is safe in the
code paths I've looked at, but why not just use

    std::cout.write(buffer->StartOfBuffer(), buffer->offset());

and then it's far easier to trust it's working?

[gabadie, 2016/02/22 14:35:46]

My bad, have always been used to use C entry point when dealing with binary
blobs. Fixed.

+  }
+  return 0;
+}
+
+// Delete a specified key from the cache.
+int DeleteKey(Backend* cache_backend, const std::string& key) {
+  net::TestCompletionCallback cb;
+  int rv = cache_backend->DoomEntry(key, cb.callback());
+  if (cb.GetResult(rv) != net::OK) {
+    std::cerr << "Couldn't delete key." << std::endl;
+    return 1;
+  }
+  return 0;
+}
+
+// Prints the command line's help.

[gavinp, 2016/02/19 16:21:11]

"Print the command line help."

[gabadie, 2016/02/22 14:35:46]

Done.

+int PrintHelp() {

[gavinp, 2016/02/19 16:21:11]

This shouldn't return a value.

[gabadie, 2016/02/22 14:35:46]

It returns 1 to have handy oneliners return PrintHelp() in main()

[gavinp, 2016/02/22 15:44:56]

That's too tricky, too ugly.

I wouldn't approve "return PrintHelp(), 1;" in a review, and I definitely won't
approve having PrintHelp unconditionally return a constant in order to make code
lower down be cute. Sorry.

+  std::cout << "cachetool <cache_path> <cache_backend_type> <subcommand> ..."

[gavinp, 2016/02/19 16:21:11]

Document available cache backend types.

[gabadie, 2016/02/22 14:35:46]

Done.

+            << std::endl
+            << std::endl;
+  std::cout << "Available subcommands:" << std::endl;
+  std::cout << "  validate: Open the cache and return." << std::endl;

[gavinp, 2016/02/19 16:21:11]

"Open a cache and return, confirming the cache exists and is of the right type"

(might even be useful to clarify that this in no way validates the structure of
the entire cache, just that it can be opened).

[gabadie, 2016/02/22 14:35:46]

Done.

+  std::cout << "  list_keys: List all cache's keys." << std::endl;

[gavinp, 2016/02/19 16:21:11]

"List all keys in a cache."

[gabadie, 2016/02/22 14:35:46]

Done.

+  std::cout << "  get_stream <key> <index>: Get key's stream." << std::endl;

[gavinp, 2016/02/19 16:21:11]

"Print a particular stream for a given key"

Also, can we use <index> symbolically here, and then document those values, i.e.
"headers" and "body" ?

[gabadie, 2016/02/22 14:35:46]

Done.

+  std::cout << "  delete_key <key>: Delete key from cache." << std::endl;
+  return 1;
+}
+
+}  // namespace
+
+int main(int argc, char* argv[]) {
+  base::AtExitManager at_exit_manager;
+  base::MessageLoopForIO message_loop;
+  base::CommandLine::Init(argc, argv);
+  const base::CommandLine& command_line =
+      *base::CommandLine::ForCurrentProcess();
+
+  base::CommandLine::StringVector args = command_line.GetArgs();
+  if (args.size() < 3U) {
+    return PrintHelp();
+  }
+
+  base::FilePath cache_path(args[0]);
+  std::string cache_backend_type(args[1]);
+  std::string subcommand(args[2]);
+
+  g_message_loop = &message_loop;
+  scoped_ptr<Backend> cache_backend;
+  if (cache_backend_type == "simple") {
+    cache_backend.reset(CreateSimpleBackend(cache_path));
+  } else {
+    std::cerr << "Unknown cache type." << std::endl;
+    return PrintHelp();
+  }
+
+  if (!cache_backend) {
+    std::cerr << "Invalid cache." << std::endl;
+    return PrintHelp();
+  }
+
+  int return_code = 1;
+  if (subcommand == "validate") {

[gavinp, 2016/02/19 16:21:11]

Why don't we check that there's only exactly three arguments for validate and
list_keys?

[gabadie, 2016/02/22 14:35:45]

Oups... Done.

+    return_code = 0;
+  } else if (subcommand == "list_keys") {
+    return_code = ListKeys(cache_backend.get());
+  } else if (subcommand == "get_stream") {
+    if (args.size() != 5) {
+      return PrintHelp();
+    }
+    std::string key(args[3]);
+    int index = std::stoi(args[4]);

[gavinp, 2016/02/19 16:21:11]

Earlier I mentioned that I'd like these to be symbolic. But also, shouldn't we
consider base::StringToInt if we are going to be parsing strings into integers?
At the very least, we should fail on errors.

[gabadie, 2016/02/22 14:35:46]

Ah sweet, I was looking into a better function in the std for detectings errors.
Thanks!

+    return_code = GetKeyStream(cache_backend.get(), key, index);
+  } else if (subcommand == "delete_key") {
+    if (args.size() != 4) {
+      return PrintHelp();
+    }
+    std::string key(args[3]);
+    return_code = DeleteKey(cache_backend.get(), key);
+  } else {
+    std::cerr << "Unknown subcommand." << std::endl;
+    PrintHelp();
+  }
+  return return_code;
+}