Multicast DNS implementation (initial)

net/dns/mdns_client_impl.h

+// 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_CLIENT_IMPL_H_
+#define NET_DNS_MDNS_CLIENT_IMPL_H_
+
+#include <map>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/cancelable_callback.h"
+#include "base/observer_list.h"
+#include "net/base/io_buffer.h"
+#include "net/base/ip_endpoint.h"
+#include "net/dns/mdns_cache.h"
+#include "net/dns/mdns_client.h"
+#include "net/udp/datagram_server_socket.h"
+#include "net/udp/udp_server_socket.h"
+#include "net/udp/udp_socket.h"
+
+
+namespace net {
+
+class MDnsListenerImpl;
+
+// This class represents a connection to the network for multicast DNS clients.

[szym, 2013/06/07 16:40:02]

nit: No need for "This class represents".

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+// It reads data into DNSResponse objects and alerts the delegate that a packet

[szym, 2013/06/07 16:40:02]

nit: DnsResponse

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+// has been received.
+class MDnsConnection {
+ public:
+  class Delegate {
+   public:
+    // Handle an mDNS packet buffered in |response| with a size of |bytes_read|.
+    virtual void HandlePacket(DnsResponse* response, int bytes_read) = 0;
+    virtual ~Delegate() {}

[szym, 2013/06/07 16:40:02]

In MDnsConnectionImpl you moved error handling to the delegate, but with the
interface above there is no way to test that path without mocking sockets.

I still think you'd better test MDnsClientImpl using mocked sockets only and
avoid adding another layer of indirection just for testing.

Even if you keep MDnsConnection and MDnsConnectionImpl separate, you will still
need to add tests for MDnsConnectionImpl using mocked sockets.

[Noam Samuel, 2013/06/07 23:54:43]

Added tests for MDnsConnectionImpl using mocked objects.

On 2013/06/07 16:40:02, szym wrote:

+  };
+
+  // Start the connection. Will begin alerting the delegate of packets.
+  virtual bool Init() = 0;

[szym, 2013/06/07 16:40:02]

nit: Returns?

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+
+  // Send a packet on both ipv4 and ipv6.
+  virtual bool Send(IOBuffer* buffer, unsigned size) = 0;

[szym, 2013/06/07 16:40:02]

nit: Returns?

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+  virtual ~MDnsConnection() {}
+};
+
+class MDnsConnectionFactory {
+ public:
+  virtual scoped_ptr<MDnsConnection> CreateConnection(
+      MDnsConnection::Delegate* delegate) = 0;
+
+  virtual ~MDnsConnectionFactory() {}
+};
+
+class MDnsClientImpl : public MDnsClient {
+ public:
+  // The core object exists while the MDnsClient is listening for MDnsPackets,
+  // and is deleted whenever the number of references for listening reachers
+  // zero. The message loop may hold weak pointers to it in order to ensure
+  // tasks scheduled on it are not delivered after it is destroyed.
+  class Core : public base::SupportsWeakPtr<Core>, MDnsConnection::Delegate {
+   public:
+    Core(MDnsClientImpl* client,
+         MDnsConnectionFactory* connection_factory);
+    virtual ~Core();
+
+    // Initialize the core. Returns true on success.
+    bool Init();
+
+    // Send a query with a specific rrtype and name. Returns true on success.
+    bool SendQuery(uint16 rrtype, std::string name);
+
+    // Add/remove a listener to the list of listener. May cause network traffic
+    // if listener is active.
+    void AddListener(MDnsListenerImpl* listener);
+    void RemoveListener(MDnsListenerImpl* listener);
+
+    // Query the cache for records of a specific type and name.
+    void QueryCache(uint16 rrtype, const std::string& name,
+                    std::vector<const RecordParsed*>* records) const;
+
+    // Parse the response and alert relevant listeners.
+    virtual void HandlePacket(DnsResponse* response, int bytes_read) OVERRIDE;
+
+   private:
+    typedef std::pair<uint16, std::string> ListenerKey;
+    typedef std::map<ListenerKey, ObserverList<MDnsListenerImpl>* >
+    ListenerMap;
+
+    // Alert listeners of an update to the cache.
+    void AlertListeners(MDnsUpdateType update_type,
+                        const ListenerKey& key, const RecordParsed* record);
+
+    // Schedule a cleanup to a specific time, cancelling other cleanups.
+    void ScheduleCleanup(base::Time cleanup);
+
+    // Clean up the cache and schedule a new cleanup.
+    void DoCleanup();
+
+    // Callback for when a record is removed from the cache.
+    void OnRecordRemoved(const RecordParsed* record);
+
+    ListenerMap listeners_;
+
+    MDnsClientImpl* client_;
+    MDnsCache cache_;
+
+    base::CancelableCallback<void()> cleanup_callback_;
+    base::Time scheduled_cleanup_;
+
+    scoped_ptr<MDnsConnection> connection_;
+
+    DISALLOW_COPY_AND_ASSIGN(Core);
+  };
+
+  MDnsClientImpl();
+
+  // Used only for testing. Lets users of this class decouple it from time
+  // dependence and network dependence.
+  explicit MDnsClientImpl(MDnsConnectionFactory* connection_factory);
+  virtual ~MDnsClientImpl();
+
+  // Add delegate for RRType |rrtype| and name |name|.
+  // If |name| is an empty string, listen to all notification of type
+  // |rrtype|.
+  virtual scoped_ptr<MDnsListener> CreateListener(
+      uint16 rrtype,
+      const std::string& name,
+      MDnsListener::Delegate* delegate) OVERRIDE;
+
+  // Create a transaction to Query MDNS for a single-value query
+  // (A, AAAA, TXT, and SRV) asynchronously. May defer to cache.
+  virtual scoped_ptr<MDnsTransaction> CreateTransaction(
+      uint16 rrtype,
+      const std::string& name,
+      int flags,
+      const MDnsTransaction::ResultCallback& callback) OVERRIDE;
+
+
+  // Functions for testing only.
+  bool IsListeningForTests();
+
+  bool AddListenRef();
+  void SubtractListenRef();
+
+  Core* core() { return core_.get(); }
+
+ private:
+  void Shutdown();
+
+  scoped_ptr<Core> core_;
+  int listen_refs_;
+
+  // Since we can either own or not own the connection_factory, use a scoped_ptr
+  // and a ptr.
+  scoped_ptr<MDnsConnectionFactory> connection_factory_owned_;
+  MDnsConnectionFactory* connection_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(MDnsClientImpl);
+};
+
+class MDnsListenerImpl : public MDnsListener,
+                         public base::SupportsWeakPtr<MDnsListenerImpl> {
+ public:
+  MDnsListenerImpl(uint16 rrtype,
+                   const std::string& name,
+                   MDnsListener::Delegate* delegate,
+                   MDnsClientImpl* client);
+
+  // Destroying the listener stops listening.
+  virtual ~MDnsListenerImpl();
+
+  // Start the listener. Returns true on success.

[szym, 2013/06/07 16:40:02]

You don't really need to copy comments on implementation of an interface.
Instead write
// MDnsListener implementation:
above the first method.

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+  virtual bool Start() OVERRIDE;
+
+  // Get the host or service name for this query.
+  // Return an empty string for no name.
+  virtual const std::string& GetName() const OVERRIDE;
+
+  // Get the type for this query (SRV, TXT, A, AAA, etc)
+  virtual uint16 GetType() const OVERRIDE;
+
+  MDnsListener::Delegate* delegate() { return delegate_; }
+
+  void AlertDelegate(MDnsUpdateType update_type,

[szym, 2013/06/07 16:40:02]

nit: Needs a comment.

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+                     const RecordParsed* record_parsed);
+ private:
+  uint16 rrtype_;
+  std::string name_;
+  MDnsClientImpl* client_;
+  MDnsListener::Delegate* delegate_;
+
+  bool started_;
+  DISALLOW_COPY_AND_ASSIGN(MDnsListenerImpl);
+};
+
+class MDnsTransactionImpl : public MDnsTransaction,
+                            public base::SupportsWeakPtr<MDnsTransactionImpl>,

[szym, 2013/06/07 16:40:02]

nit: Don't put SupportsWeakPtr in between two interfaces. I suggest making it
first.

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+                            public MDnsListener::Delegate {
+ public:
+  MDnsTransactionImpl(uint16 rrtype,
+                      const std::string& name,
+                      int flags,
+                      const MDnsTransaction::ResultCallback& callback,
+                      MDnsClientImpl* client);
+  virtual ~MDnsTransactionImpl();
+
+  // Start the transaction. Returns true on success.

[szym, 2013/06/07 16:40:02]

// MDnsTransaction implementation:

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+  virtual bool Start() OVERRIDE;
+
+  // MDnsListener::Delegate implementation
+  virtual void OnRecordUpdate(MDnsUpdateType update,
+                              const RecordParsed* record) OVERRIDE;
+  virtual void OnNsecRecord(const std::string& name, unsigned type) OVERRIDE;
+
+  virtual void OnCachePurged() OVERRIDE;
+
+  virtual const std::string& GetName() const OVERRIDE;

[szym, 2013/06/07 16:40:02]

Keep the methods from different interfaces separate. MDnsTransaction before
MDnsListener::Delegate

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+  virtual uint16 GetType() const OVERRIDE;
+
+  bool is_valid() { return !callback_.is_null(); }

[szym, 2013/06/07 16:40:02]

I'd suggest is_active() or is_pending(), since nothing is being validated.

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+
+  void Reset();
+
+ private:
+  // Trigger the callback and reset all related variables.
+  void TriggerCallback(MDnsTransactionResult result,
+                       const RecordParsed* record);
+
+  // Internal callback for when a cache record is found.
+  void CacheRecordFound(const RecordParsed* record);
+
+  // Signal the transactionis over and release all related resources.
+  void SignalTransactionOver();
+
+  uint16 rrtype_;
+  std::string name_;
+  MDnsTransaction::ResultCallback callback_;
+
+  scoped_ptr<MDnsListener> listener_;
+  base::CancelableCallback<void()> timeout_;
+
+  MDnsClientImpl* client_;
+
+  bool started_;
+  int flags_;
+
+  DISALLOW_COPY_AND_ASSIGN(MDnsTransactionImpl);
+};
+
+// This implementation of MDnsConnection handles connecting to both IPv4 and
+// IPv6.
+class MDnsConnectionImpl : public MDnsConnection {
+ public:
+  explicit MDnsConnectionImpl(MDnsConnection::Delegate* delegate);
+  virtual ~MDnsConnectionImpl();
+
+  virtual bool Init() OVERRIDE;
+  virtual bool Send(IOBuffer* buffer, unsigned size) OVERRIDE;
+
+ private:
+  class RecvLoop {

[szym, 2013/06/07 16:40:02]

This could be better structured, by moving socket ownership, BindSocket, and
Send to RecvLoop. It should also have the IPEndPoint stored in a field.
MDnsConnectionImpl::Send would just forward the arguments to RecvLoop::Send.

[Noam Samuel, 2013/06/07 23:54:43]

Done.

+   public:
+    RecvLoop(DatagramServerSocket* socket, MDnsConnectionImpl* connection);
+    ~RecvLoop();
+    void DoLoop(int rv);
+
+   private:
+    void OnDatagramReceived(int rv);
+
+    DatagramServerSocket* socket_;
+    MDnsConnectionImpl* connection_;
+    IPEndPoint recv_addr_;
+    scoped_ptr<DnsResponse> response_;
+  };
+  // Bind a socket with a specific address size to a specific multicast group
+  // and port 5353.
+  bool BindSocket(DatagramServerSocket* socket,
+                  int addr_size,
+                  const char* multicast_group);
+
+  // Callback for handling a datagram being recieved on either ipv4 or ipv6.
+  // Responsible for ensuring we request another packet from the network.
+  void OnDatagramReceived(DatagramServerSocket* socket,
+                          DnsResponse* response,
+                          IPEndPoint* recv_addr,
+                          int bytes_read);
+
+  void OnError(RecvLoop* loop, DatagramServerSocket* socket, int error);
+
+  // Callback for when sending a query has finished.
+  void SendDone(int sent);
+
+  // The IPEndPoints for sending/recieving packets on IPv4 and IPv6.
+  IPEndPoint GetIPv4SendEndpoint();
+  IPEndPoint GetIPv6SendEndpoint();
+
+  scoped_ptr<DatagramServerSocket> socket_ipv4_;
+  scoped_ptr<DatagramServerSocket> socket_ipv6_;
+
+  RecvLoop loop_ipv4_;
+  RecvLoop loop_ipv6_;
+
+  MDnsConnection::Delegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(MDnsConnectionImpl);
+};
+
+class MDnsConnectionImplFactory : public MDnsConnectionFactory {
+ public:
+  MDnsConnectionImplFactory();
+  virtual ~MDnsConnectionImplFactory();
+
+  virtual scoped_ptr<MDnsConnection> CreateConnection(
+      MDnsConnection::Delegate* delegate) OVERRIDE;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MDnsConnectionImplFactory);
+};
+
+}
+#endif  // NET_DNS_MDNS_CLIENT_IMPL_H_