On Mon, Apr 03, 2017 at 04:10:15PM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique <nusid...@redhat.com>
> 
> This patch adds a new OVN action 'dns_lookup' to support native DNS.
> ovn-controller parses this action and adds a NXT_PACKET_IN2
> OF flow with 'pause' flag set.
> 
> A new table 'DNS' is added in the SB DB to look up and resolve
> the DNS queries. When a valid DNS packet is received by
> ovn-controller, it looks up the DNS name in the 'DNS' table
> and if successful, it frames a DNS reply, resumes the packet
> and stores 1 in the 1-bit subfield. If the packet is invalid
> or cannot be resolved, it resumes the packet without any
> modifications and stores 0 in the 1-bit subfield.
> 
> reg0[4] = dns_lookup(); next;
> 
> An upcoming patch will use this action and adds logical flows.
> 
> Signed-off-by: Numan Siddique <nusid...@redhat.com>

Thanks for revising this!  DNS is important and it goes one step closer
to offering everything that OpenStack needs from OVN.

I think that Guru is planning to review this too, particularly regarding
the database schema (and its documentation, I hope), but I'll offer some
preliminary comments of my own.

I tend to think that the DNS protocol declarations should go in
lib/packets.h, where OVS generally keeps protocol declarations, instead
of ovn/lib/ovn-util.h.

I think that the ovn-trace implementation of dns_lookup should at least
set the destination bit to 0, to indicate that the lookup failed.

I suggest folding in the following incremental to make the code easier
to read.  (I haven't tested it.)

Thanks,

Ben.

--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 9c669c96884a..e349817e7f4c 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -662,6 +662,18 @@ exit:
 }
 
 static void
+put_be16(struct ofpbuf *buf, ovs_be16 x)
+{
+    ofpbuf_put(buf, &x, sizeof x);
+}
+
+static void
+put_be32(struct ofpbuf *buf, ovs_be32 x)
+{
+    ofpbuf_put(buf, &x, sizeof x);
+}
+
+static void
 pinctrl_handle_dns_lookup(
     struct dp_packet *pkt_in, struct ofputil_packet_in *pin,
     struct ofpbuf *userdata, struct ofpbuf *continuation,
@@ -722,27 +734,19 @@ pinctrl_handle_dns_lookup(
     /* Extract the hostname. If the hostname is - 'www.ovn.org' it would be
      * encoded as (in hex) - 03 77 77 77 03 6f 76 63 03 6f 72 67 00.
      */
-    bool hostname_present = true;
     while ((in_dns_data + idx) < end && in_dns_data[idx]) {
         uint8_t label_len = in_dns_data[idx++];
         if (in_dns_data + idx + label_len > end) {
-            hostname_present = false;
-            break;
-        }
-        for (uint8_t i = 0; i < label_len; i++) {
-            ds_put_char(&hostname, in_dns_data[idx++]);
+            ds_destroy(&hostname);
+            goto exit;
         }
+        ds_put_buffer(&hostname, (const char *) in_dns_data + idx, label_len);
+        idx += label_len;
         ds_put_char(&hostname, '.');
     }
 
-    if (!hostname_present) {
-        ds_destroy(&hostname);
-        goto exit;
-    }
-
     idx++;
     ds_chomp(&hostname, '.');
-    ds_put_char(&hostname, 0);
     in_dns_data += idx;
 
     /* Query should have TYPE and CLASS fields */
@@ -809,35 +813,25 @@ pinctrl_handle_dns_lookup(
              *                be IP address of the domain name.
              */
             ofpbuf_put(&dns_answer, in_hostname, idx);
-            ovs_be16 v = htons(DNS_QUERY_TYPE_A);
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            v = htons(DNS_CLASS_IN);
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            ovs_be32 ttl = htonl(DNS_DEFAULT_RR_TTL);
-            ofpbuf_put(&dns_answer, &ttl, sizeof(ovs_be32));
-            v = htons(sizeof(ovs_be32)); /* Length of the RDATA field */
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            ofpbuf_put(&dns_answer, &ip_addrs.ipv4_addrs[i].addr,
-                       sizeof(ovs_be32));
+            put_be16(&dns_answer, htons(DNS_QUERY_TYPE_A));
+            put_be16(&dns_answer, htons(DNS_CLASS_IN));
+            put_be32(&dns_answer, htonl(DNS_DEFAULT_RR_TTL));
+            put_be16(&dns_answer, htons(sizeof(ovs_be32)));
+            put_be32(&dns_answer, ip_addrs.ipv4_addrs[i].addr);
             ancount++;
         }
     }
 
     if (query_type == DNS_QUERY_TYPE_AAAA ||
-            query_type == DNS_QUERY_TYPE_ANY) {
+        query_type == DNS_QUERY_TYPE_ANY) {
         for (size_t i = 0; i < ip_addrs.n_ipv6_addrs; i++) {
             ofpbuf_put(&dns_answer, in_hostname, idx);
-            ovs_be16 v = htons(DNS_QUERY_TYPE_AAAA);
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            v = htons(DNS_CLASS_IN);
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            ovs_be32 ttl = htonl(DNS_DEFAULT_RR_TTL);
-            ofpbuf_put(&dns_answer, &ttl, sizeof(ovs_be32));
-            /* Length of the RDATA field */
-            v = htons(sizeof(ip_addrs.ipv6_addrs[i].addr.s6_addr));
-            ofpbuf_put(&dns_answer, &v, sizeof(ovs_be16));
-            ofpbuf_put(&dns_answer, &ip_addrs.ipv6_addrs[i].addr.s6_addr,
-                       sizeof(ip_addrs.ipv6_addrs[i].addr.s6_addr));
+            put_be16(&dns_answer, htons(DNS_QUERY_TYPE_AAAA));
+            put_be16(&dns_answer, htons(DNS_CLASS_IN));
+            put_be32(&dns_answer, htonl(DNS_DEFAULT_RR_TTL));
+            const struct in6_addr *ip6 = &ip_addrs.ipv6_addrs[i].addr;
+            put_be16(&dns_answer, htons(sizeof *ip6));
+            ofpbuf_put(&dns_answer, ip6, sizeof *ip6);
             ancount++;
         }
     }
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to