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..8cc7b7dbbaf8466f95b4d96373452e72d98ff47b
--- /dev/null
+++ b/net/dns/mdns_cache.h
@@ -0,0 +1,127 @@
+// 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 <list>
+#include <map>
+#include <string>
+
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time.h"
+#include "net/base/net_export.h"
+
+namespace net {
+
+class ParsedDnsRecord;
+
+class RecordParsed;
+
+class NET_EXPORT_PRIVATE MDnsCache {

[szym, 2013/05/14 18:14:27]

Needs class-level or file-level comment.

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+ public:
+  enum UpdateType {
+    RecordAdded,
+    RecordChanged,
+    RecordRemoved,
+    NoChange
+  };
+
+  class DnsRecordCacheKey {

[szym, 2013/05/14 18:14:27]

Needs comment here or at the file-level.

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+   public:
+    DnsRecordCacheKey(uint16 type,
+                      const std::string& name,
+                      const std::string& optional);
+
+    ~DnsRecordCacheKey();
+    bool operator<(const DnsRecordCacheKey& key) const;
+    bool operator==(const DnsRecordCacheKey& key) const;
+
+    uint16 type() const { return type_; }
+    const std::string& name() const  { return name_; }
+    const std::string& optional() const { return optional_; }
+
+   private:
+     uint16 type_;

[szym, 2013/05/14 18:14:27]

Does it really need to be uint16? Why not unsigned?

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+     std::string name_;
+     std::string optional_;
+  };
+
+  class Query {

[szym, 2013/05/14 18:14:27]

Comment?

It seems to me that this implements the STL predicate design pattern. I think
it'd make sense to just use it directly with std::find_if.

[Noam Samuel, 2013/05/14 19:28:01]

It's slightly different because it assumes records are sorted and that all
records matching a certain query are contiguous. In practice, the uses of this
are fairly narrow. This is basically just a wrapper for either type/name query
or type-only query. 

In hindsight, I think it might be better to do away with this class and just
write a function to query the cache for records of a certain type and optionally
name (with an empty name implying all records of a certain type). This class
structure is far too general-looking, when in reality it has very limited use
cases.

On 2013/05/14 18:14:27, szym wrote:

[Noam Samuel, 2013/05/15 00:11:31]

Changed querying to remove this class.

+   public:
+    virtual ~Query() {}
+
+    virtual DnsRecordCacheKey GetLowerBoundKey() const = 0;
+    virtual bool IsMatching(const DnsRecordCacheKey& key) const = 0;
+  };
+
+  class TypeOnlyQuery : public MDnsCache::Query {
+   public:
+    explicit TypeOnlyQuery(uint16 type);
+    virtual ~TypeOnlyQuery();
+
+    virtual DnsRecordCacheKey GetLowerBoundKey() const OVERRIDE;
+    virtual bool IsMatching(const DnsRecordCacheKey& key) const OVERRIDE;
+   private:
+    uint16 type_;
+  };
+
+  class TypeNameQuery : public MDnsCache::Query {
+   public:
+    TypeNameQuery(uint16 type, const std::string& name);
+    virtual ~TypeNameQuery();
+
+    virtual DnsRecordCacheKey GetLowerBoundKey() const OVERRIDE;
+    virtual bool IsMatching(const DnsRecordCacheKey& key) const OVERRIDE;
+   private:
+    uint16 type_;
+    std::string name_;
+  };
+
+  MDnsCache();
+  ~MDnsCache();
+
+  // Update the cache with the new record. Takes ownership of the record.
+  UpdateType UpdateDnsRecord(const RecordParsed* record);

[szym, 2013/05/14 18:14:27]

Use scoped_ptr<RecordParsed> instead of the comment about ownership.

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+
+  // Return false if nothing has been found. Expired records will not be
+  // returned.
+  bool FindDnsRecords(const Query& query,
+                      std::list<const RecordParsed*>* results,

[szym, 2013/05/14 18:14:27]

If you always return |results| why do you need to return bool? Is
results->empty() not enough?

Do you definitely need a linked list? Why not a vector?

If you used find_if, you could return an iterator, but I think a vector would
work fine.

[Noam Samuel, 2013/05/14 19:28:01]

Hm. I think you're right that ideally an iterator would be better. As mentioned
above, find_if doesn't really make sense in this context. However, it might be
beneficial to just write an iterator.

On 2013/05/14 18:14:27, szym wrote:

[Noam Samuel, 2013/05/15 00:11:31]

On second thought, writing an iterator will probably be overkill for this. If
you think it will be beneficial I can write one, but my intuition from starting
on it is that it will introduce more complexity than it's worth.

+                      base::Time now);
+
+  // |records_removed| will be populated with the removed records. Uses
+  // linked_ptr to signify that the ownership is being transferred to the
+  // user.
+  void CleanupRecords(
+      std::list<linked_ptr<const RecordParsed> >* records_removed,

[szym, 2013/05/14 18:14:27]

Ditto.

[Noam Samuel, 2013/05/15 00:11:31]

Made irrelevant.

+      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.
+  base::Time GetNextExpiration() const { return next_expiration_; }

[szym, 2013/05/14 18:14:27]

If it's a simple accessor, code style says it should be "next_expiration()".

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+
+  void Clear();
+
+ private:
+  base::Time GetEffectiveExpiration(const RecordParsed* entry);

[szym, 2013/05/14 18:14:27]

Needs comment.

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+  std::string GetOptionalFieldForRecord(const RecordParsed* record);

[szym, 2013/05/14 18:14:27]

Ditto.

[Noam Samuel, 2013/05/15 00:11:31]

Done.

+
+  typedef std::map<DnsRecordCacheKey, linked_ptr<const RecordParsed> >
+      MDnsCacheMap;
+
+  MDnsCacheMap mdns_cache_;
+
+  // next_expiration_ is always guaranteed to be less than or equal to the next
+  // expiration. A value of base::Time() implies there is no next expiration
+  // (empty cache).
+  base::Time next_expiration_;
+
+  DISALLOW_COPY_AND_ASSIGN(MDnsCache);
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_MDNS_CACHE_H_