Cache for mDNS records

net/dns/mdns_cache.h

Index: net/dns/mdns_cache.h
diff --git a/net/dns/mdns_cache.h b/net/dns/mdns_cache.h
new file mode 100644
index 0000000000000000000000000000000000000000..5196747ffbb4b8c1e1a7df5b3bc17ab27e366396
--- /dev/null
+++ b/net/dns/mdns_cache.h
@@ -0,0 +1,120 @@
+// Copyright (c) 2013 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.
+
+#ifndef NET_DNS_MDNS_CACHE_H_
+#define NET_DNS_MDNS_CACHE_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "net/base/net_export.h"
+
+/**

[szym, 2013/05/17 05:04:34]

Chromium C++ style says use // comments only.

I'd also suggest moving this to just above the class.

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+ * mDNS Cache
+ *
+ * This is a cache of mDNS records. It keeps track of expiration times and is
+ * guaranteed not to return expired records. It also has facilities for timely
+ * record expiration.
+ */
+
+namespace net {
+
+class ParsedDnsRecord;
+

[szym, 2013/05/17 05:04:34]

nit: remove blank line

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+class RecordParsed;
+
+class NET_EXPORT_PRIVATE MDnsCache {
+ public:
+  typedef base::Callback<void(const RecordParsed*)> RecordRemovedCallback;
+
+  enum UpdateType {
+    RecordAdded,
+    RecordChanged,
+    RecordRemoved,

[szym, 2013/05/17 05:04:34]

RecordRemoved is never used. Do you need it?

[Noam Samuel, 2013/05/17 17:21:02]

Hm. I added it here so I could use it to signify update types in the MDns layer,
but considering that its use is overloaded, perhaps it is better to pull it out
and put it somewhere else, such as a common header. I'm just not sure where to
put it short of making an "mdns_common" header. 

+    NoChange
+  };
+
+  // |record_removed_callback| will be called whenever a record is removed.
+  // The calls are done synchronously.

[szym, 2013/05/17 05:04:34]

|record_removed_callback| is called in CleanupRecords only, so I'd pass it to
CleanupRecords rather than making it a permanent property of the cache. If you
keep it as is, explain here that the callback is called in CleanupRecords only
(as opposed to Clear or destructor).

[Noam Samuel, 2013/05/17 22:26:05]

|record_removed_callback| now passed to CleanupRecords.

On 2013/05/17 05:04:34, szym wrote:

+  explicit MDnsCache(const RecordRemovedCallback& record_removed_callback);
+  ~MDnsCache();
+
+  // Return value indicates whether the record was added, changed
+  // (existed previously with different value) or not changed (existed
+  // previously with same value).
+  UpdateType UpdateDnsRecord(scoped_ptr<const RecordParsed> record);
+
+  // Return records with type |type| and name |name|. Expired records will not
+  // be returned. If |name| is empty, return all records with type |type|.
+  void FindDnsRecords(unsigned type,
+                      const std::string& name,
+                      std::vector<const RecordParsed*>* records,
+                      base::Time now);
+
+  // Empty the cache, call |record_removed_callback_| for every removed record.

[szym, 2013/05/17 05:04:34]

Do you really mean "empty the cache", or is it "remove expired records"?

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+  void CleanupRecords(base::Time now);
+
+  // Returns a time less than or equal to the next time a record will expire.
+  // Is updated when CleapRecords or UpdateDnsRecord are called.

[szym, 2013/05/17 05:04:34]

Fix typo. Should explain returns base::Time() when empty.

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+  base::Time next_expiration() const { return next_expiration_; }
+
+  void Clear();
+
+ private:
+  // Key type for the record map. It is a 3-tuple of type, name and optional
+  // value ordered by type, then name, then optional value. This allows us to
+  // query for all records of a certain type and name, while also allowing us
+  // to set records of a certain type, name and optionally value as unique.
+  class DnsRecordCacheKey {
+   public:
+    DnsRecordCacheKey(unsigned type,
+                      const std::string& name,
+                      const std::string& optional);
+

[szym, 2013/05/17 05:04:34]

nit: remove blank

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+    ~DnsRecordCacheKey();
+    bool operator<(const DnsRecordCacheKey& key) const;
+    bool operator==(const DnsRecordCacheKey& key) const;
+

[szym, 2013/05/17 05:04:34]

I believe this needs explicit copy constructor and operator=, but I'd settle for
a comment that the implicit ones are used on purpose.

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+    unsigned type() const { return type_; }
+    const std::string& name() const  { return name_; }
+    const std::string& optional() const { return optional_; }
+
+   private:
+     unsigned type_;
+     std::string name_;
+     std::string optional_;
+  };
+
+  typedef std::map<DnsRecordCacheKey, const RecordParsed*>
+      MDnsCacheMap;
+
+  // Get the effective expiration of a cache entry, based on its creation time
+  // and ttl. Does adjustments so entries with a TTL of zero will have a
+  // nonzero ttl, as explained in RFC 6762 Section 10.1.

[szym, 2013/05/17 05:04:34]

nit: Use "TTL" consistently (not "ttl").

[Noam Samuel, 2013/05/17 22:26:05]

Done.

+  base::Time GetEffectiveExpiration(const RecordParsed* entry);
+
+  // Get optional part of the DNS key for shared records. For example, in PTR
+  // records this is the pointed domain, since multiple PTR records may exist
+  // for the same name.
+  std::string GetOptionalFieldForRecord(const RecordParsed* record);
+
+  MDnsCacheMap mdns_cache_;
+
+  // next_expiration_ is always guaranteed to be less than or equal to the next

[szym, 2013/05/17 05:04:34]

nit: Wrap references to variables in ||.

To avoid redundant comments, comment either at the accessor or here.

[Noam Samuel, 2013/05/17 22:26:05]

Removed comment here. Better to have comment in publicly-facing accessor.

On 2013/05/17 05:04:34, szym wrote:

+  // expiration. A value of base::Time() implies there is no next expiration
+  // (empty cache).
+  base::Time next_expiration_;
+
+  RecordRemovedCallback record_removed_callback_;

[szym, 2013/05/17 05:04:34]

If you keep it and don't intend to change it, make const.

[Noam Samuel, 2013/05/17 22:26:05]

Removed.

On 2013/05/17 05:04:34, szym wrote:

+
+  DISALLOW_COPY_AND_ASSIGN(MDnsCache);
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_MDNS_CACHE_H_