diff mbox

[ovs-dev,PATCHv1,1/4] ovn-northd: Support Logical_Port.addresses to store multiple ips in each set

Message ID 56AB8BED.4070203@redhat.com
State Changes Requested
Headers show

Commit Message

Numan Siddique Jan. 29, 2016, 3:57 p.m. UTC
If a logical port has two ipv4 addresses and one ipv6 address
it will be stored as ["MAC IPv41 IPv42 IPv61"] instead of
["MAC IPv41", "MAC IPv42", "MAC IPv61"].

Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
---
 lib/packets.c           | 102 ++++++++++++++++++++++++++++++++++
 lib/packets.h           |  10 ++++
 ovn/northd/ovn-northd.c | 142 ++++++++++++++++++++++++++++++++++++++++++------
 ovn/ovn-nb.xml          |  58 +++++++++++++++-----
 4 files changed, 280 insertions(+), 32 deletions(-)

Comments

Ben Pfaff Feb. 20, 2016, 2:37 a.m. UTC | #1
On Fri, Jan 29, 2016 at 09:27:33PM +0530, Numan Siddique wrote:
> If a logical port has two ipv4 addresses and one ipv6 address
> it will be stored as ["MAC IPv41 IPv42 IPv61"] instead of
> ["MAC IPv41", "MAC IPv42", "MAC IPv61"].
> 
> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

This appears at first to add a lot of duplicate code in packets.c.  Can,
perhaps, the existing parsing functions be written in terms of the new
functions?  It would be better to avoid having a lot of cut-and-paste
code if we can.

extract_lport_addresses() appears to just silently ignore errors.  It
would be better to log them (rate-limited) so that administrators have a
chance to catch misconfiguration.

The description in ovn-nb.xml implies that IPv4 and IPv6 addresses can
be mixed together, but the implementation in extract_lport_addresses()
requires IPv4 addresses to precede IPv6 addresses.  It would be better
to allow the freer form.

The code appears to parse IPv6 addresses and then completely ignore
them.

Code of the form
        if (p) {
            free(p);
        }
can be simply written
        free(p);

extract_lport_addresses() does a lot of malloc()s.  At the very least,
the malloc of struct lport_addresses itself is unnecessary: the caller
can simply provide one on the stack, by pointer.

Thanks,

Ben.
Numan Siddique Feb. 22, 2016, 6:38 a.m. UTC | #2
On 02/20/2016 08:07 AM, Ben Pfaff wrote:
> On Fri, Jan 29, 2016 at 09:27:33PM +0530, Numan Siddique wrote:
>> If a logical port has two ipv4 addresses and one ipv6 address
>> it will be stored as ["MAC IPv41 IPv42 IPv61"] instead of
>> ["MAC IPv41", "MAC IPv42", "MAC IPv61"].
>>
>> Signed-off-by: Numan Siddique <nusiddiq@redhat.com>
> This appears at first to add a lot of duplicate code in packets.c.  Can,
> perhaps, the existing parsing functions be written in terms of the new
> functions?  It would be better to avoid having a lot of cut-and-paste
> code if we can.

Sure. I will do it.

>
> extract_lport_addresses() appears to just silently ignore errors.  It
> would be better to log them (rate-limited) so that administrators have a
> chance to catch misconfiguration.

Done.

>
> The description in ovn-nb.xml implies that IPv4 and IPv6 addresses can
> be mixed together, but the implementation in extract_lport_addresses()
> requires IPv4 addresses to precede IPv6 addresses.  It would be better
> to allow the freer form.

Sure. I will do it that way.
>
> The code appears to parse IPv6 addresses and then completely ignore
> them.
>
> Code of the form
>         if (p) {
>             free(p);
>         }
> can be simply written
>         free(p);
>
> extract_lport_addresses() does a lot of malloc()s.  At the very least,
> the malloc of struct lport_addresses itself is unnecessary: the caller
> can simply provide one on the stack, by pointer.

Sure. I will do it as per your comments.

Thanks for reviewing.

> Thanks,
>
> Ben.
diff mbox

Patch

diff --git a/lib/packets.c b/lib/packets.c
index d82341d..b338846 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -455,6 +455,34 @@  ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
     return NULL;
 }
 
+/*
+ * This function is similar to ip_parse_masked(), with an extra parameter
+ * `n` added to return the number of scanned characters.
+ */
+char * OVS_WARN_UNUSED_RESULT
+ip_parse_masked_len(const char *s, int *n, ovs_be32 *ip,
+                    ovs_be32 *mask)
+{
+    int prefix;
+
+    if (ovs_scan_len(s, n, IP_SCAN_FMT"/"IP_SCAN_FMT,
+                 IP_SCAN_ARGS(ip), IP_SCAN_ARGS(mask))) {
+        /* OK. */
+    } else if (ovs_scan_len(s, n, IP_SCAN_FMT"/%d",
+                            IP_SCAN_ARGS(ip), &prefix)) {
+        if (prefix <= 0 || prefix > 32) {
+            return xasprintf("%s: network prefix bits not between 0 and "
+                             "32", s);
+        }
+        *mask = be32_prefix_mask(prefix);
+    } else if (ovs_scan_len(s, n, IP_SCAN_FMT, IP_SCAN_ARGS(ip))) {
+        *mask = OVS_BE32_MAX;
+    } else {
+        return xasprintf("%s: invalid IP address", s);
+    }
+    return NULL;
+}
+
 /* Similar to ip_parse_masked(), but the mask, if present, must be a CIDR mask
  * and is returned as a prefix length in '*plen'. */
 char * OVS_WARN_UNUSED_RESULT
@@ -475,6 +503,26 @@  ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
     return NULL;
 }
 
+/* Similar to ip_parse_cidr(), with an extra parameter `n` added to return
+ * the number of scanned characters. */
+char * OVS_WARN_UNUSED_RESULT
+ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip, unsigned int *plen)
+{
+    ovs_be32 mask;
+    char *error;
+
+    error = ip_parse_masked_len(s, n, ip, &mask);
+    if (error) {
+        return error;
+    }
+
+    if (!ip_is_cidr(mask)) {
+        return xasprintf("%s: CIDR network required", s);
+    }
+    *plen = ip_count_cidr_bits(mask);
+    return NULL;
+}
+
 /* Parses string 's', which must be an IPv6 address.  Stores the IPv6 address
  * into '*ip'.  Returns true if successful, otherwise false. */
 bool
@@ -519,6 +567,39 @@  ipv6_parse_masked(const char *s, struct in6_addr *ip, struct in6_addr *mask)
     return xasprintf("%s: invalid IPv6 address", s);
 }
 
+/*
+ * This function is similar to ipv6_parse_masked(), with an extra parameter
+ * `n` added to return the number of scanned characters.
+ */
+char * OVS_WARN_UNUSED_RESULT
+ipv6_parse_masked_len(const char *s, int *n, struct in6_addr *ip,
+                      struct in6_addr *mask)
+{
+    char ipv6_s[IPV6_SCAN_LEN + 1];
+    int prefix;
+
+    if (ovs_scan_len(s, n, " "IPV6_SCAN_FMT, ipv6_s)
+        && ipv6_parse(ipv6_s, ip)) {
+        if (ovs_scan_len(s, n, "/%d", &prefix)) {
+            if (prefix <= 0 || prefix > 128) {
+                return xasprintf("%s: IPv6 network prefix bits not between 0 "
+                                 "and 128", s);
+            }
+            *mask = ipv6_create_mask(prefix);
+        } else if (ovs_scan_len(s, n, "/"IPV6_SCAN_FMT, ipv6_s)) {
+             if (!ipv6_parse(ipv6_s, mask)) {
+                 return xasprintf("%s: Invalid IPv6 mask", s);
+             }
+            /* OK. */
+        } else {
+            /* OK. No mask */
+            *mask = in6addr_exact;
+        }
+        return NULL;
+    }
+    return xasprintf("%s: invalid IPv6 address", s);
+}
+
 /* Similar to ipv6_parse_masked(), but the mask, if present, must be a CIDR
  * mask and is returned as a prefix length in '*plen'. */
 char * OVS_WARN_UNUSED_RESULT
@@ -539,6 +620,27 @@  ipv6_parse_cidr(const char *s, struct in6_addr *ip, unsigned int *plen)
     return NULL;
 }
 
+/* Similar to ipv6_parse_cidr(), with an extra parameter `n` added to return
+ * the number of scanned characters. */
+char * OVS_WARN_UNUSED_RESULT
+ipv6_parse_cidr_len(const char *s, int *n, struct in6_addr *ip,
+                    unsigned int *plen)
+{
+    struct in6_addr mask;
+    char *error;
+
+    error = ipv6_parse_masked_len(s, n, ip, &mask);
+    if (error) {
+        return error;
+    }
+
+    if (!ipv6_is_cidr(&mask)) {
+        return xasprintf("%s: IPv6 CIDR network required", s);
+    }
+    *plen = ipv6_count_cidr_bits(&mask);
+    return NULL;
+}
+
 /* Stores the string representation of the IPv6 address 'addr' into the
  * character array 'addr_str', which must be at least INET6_ADDRSTRLEN
  * bytes long. */
diff --git a/lib/packets.h b/lib/packets.h
index 834e8a4..31a4e92 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -586,6 +586,11 @@  char *ip_parse_masked(const char *s, ovs_be32 *ip, ovs_be32 *mask)
     OVS_WARN_UNUSED_RESULT;
 char *ip_parse_cidr(const char *s, ovs_be32 *ip, unsigned int *plen)
     OVS_WARN_UNUSED_RESULT;
+char *ip_parse_masked_len(const char *s, int *n, ovs_be32 *ip, ovs_be32 *mask)
+    OVS_WARN_UNUSED_RESULT;
+char *ip_parse_cidr_len(const char *s, int *n, ovs_be32 *ip,
+                        unsigned int *plen)
+    OVS_WARN_UNUSED_RESULT;
 
 #define IP_VER(ip_ihl_ver) ((ip_ihl_ver) >> 4)
 #define IP_IHL(ip_ihl_ver) ((ip_ihl_ver) & 15)
@@ -1033,6 +1038,11 @@  char *ipv6_parse_masked(const char *s, struct in6_addr *ipv6,
                         struct in6_addr *mask);
 char *ipv6_parse_cidr(const char *s, struct in6_addr *ip, unsigned int *plen)
     OVS_WARN_UNUSED_RESULT;
+char *ipv6_parse_masked_len(const char *s, int *n, struct in6_addr *ipv6,
+                            struct in6_addr *mask);
+char *ipv6_parse_cidr_len(const char *s, int *n, struct in6_addr *ip,
+                          unsigned int *plen)
+    OVS_WARN_UNUSED_RESULT;
 
 void *eth_compose(struct dp_packet *, const struct eth_addr eth_dst,
                   const struct eth_addr eth_src, uint16_t eth_type,
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 4f03287..f20f3b1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -914,6 +914,96 @@  ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow)
     }
 }
 
+struct ipv4_netaddr {
+    ovs_be32 addr;
+    unsigned int plen;
+};
+
+struct ipv6_netaddr {
+    struct in6_addr addr;
+    unsigned int plen;
+};
+
+struct lport_addresses {
+    struct eth_addr ea;
+    size_t n_ipv4_addrs;
+    struct ipv4_netaddr *ipv4_addrs;
+    size_t n_ipv6_addrs;
+    struct ipv6_netaddr *ipv6_addrs;
+};
+
+/*
+ * Extracts the mac, ipv4 and ipv6 addresses from the input
+ * param 'address' which should be of the format
+ * 'MAC [IPV41 IPV42 ...IPV61 IPV62 .."
+ */
+static struct lport_addresses *
+extract_lport_addresses(char *address)
+{
+    struct lport_addresses *laddrs;
+    int n = 0;
+    char *p = address;
+    laddrs = xzalloc(sizeof (*laddrs));
+    if (!ovs_scan_len(p, &n, ETH_ADDR_SCAN_FMT,
+                      ETH_ADDR_SCAN_ARGS(laddrs->ea))) {
+        free(laddrs);
+        return NULL;
+    }
+
+    ovs_be32 ip4;
+    unsigned int plen;
+    int n1 = n;
+    int i = 0;
+    char *error;
+    while (!(error = ip_parse_cidr_len(p, &n1, &ip4, &plen))) {
+        laddrs->n_ipv4_addrs++;
+    }
+
+    if (error) {
+        free(error);
+    }
+
+    struct in6_addr ip6;
+    while (!(error = ipv6_parse_cidr_len(p, &n1, &ip6, &plen))) {
+         laddrs->n_ipv6_addrs++;
+    }
+
+    if (error) {
+        free(error);
+    }
+
+    n1 = n;
+    if (laddrs->n_ipv4_addrs) {
+        laddrs->ipv4_addrs = xcalloc(laddrs->n_ipv4_addrs,
+                                     sizeof (struct ipv4_netaddr));
+        i = 0;
+        while (!(error = ip_parse_cidr_len(p, &n1, &laddrs->ipv4_addrs[i].addr,
+                                           &laddrs->ipv4_addrs[i].plen))) {
+            i++;
+        }
+
+        if (error) {
+            free(error);
+        }
+    }
+
+    if (laddrs->n_ipv6_addrs) {
+        laddrs->ipv6_addrs = xcalloc(laddrs->n_ipv6_addrs,
+                                     sizeof (struct ipv6_netaddr));
+        i = 0;
+        while (!(error = ipv6_parse_cidr_len(
+            p, &n1, &laddrs->ipv6_addrs[i].addr,
+            &laddrs->ipv6_addrs[i].plen))) {
+            i++;
+        }
+
+        if (error) {
+            free(error);
+        }
+    }
+    return laddrs;
+}
+
 /* Appends port security constraints on L2 address field 'eth_addr_field'
  * (e.g. "eth.src" or "eth.dst") to 'match'.  'port_security', with
  * 'n_port_security' elements, is the collection of port_security constraints
@@ -1194,14 +1284,15 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
         }
 
         for (size_t i = 0; i < op->nbs->n_addresses; i++) {
-            struct eth_addr ea;
-            ovs_be32 ip;
-
-            if (ovs_scan(op->nbs->addresses[i],
-                         ETH_ADDR_SCAN_FMT" "IP_SCAN_FMT,
-                         ETH_ADDR_SCAN_ARGS(ea), IP_SCAN_ARGS(&ip))) {
+            struct lport_addresses *laddrs = extract_lport_addresses(
+                op->nbs->addresses[i]);
+            if (!laddrs) {
+                continue;
+            }
+            for (size_t j = 0; j < laddrs->n_ipv4_addrs; j++) {
                 char *match = xasprintf(
-                    "arp.tpa == "IP_FMT" && arp.op == 1", IP_ARGS(ip));
+                    "arp.tpa == "IP_FMT" && arp.op == 1",
+                    IP_ARGS(laddrs->ipv4_addrs[j].addr));
                 char *actions = xasprintf(
                     "eth.dst = eth.src; "
                     "eth.src = "ETH_ADDR_FMT"; "
@@ -1213,14 +1304,22 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
                     "outport = inport; "
                     "inport = \"\"; /* Allow sending out inport. */ "
                     "output;",
-                    ETH_ADDR_ARGS(ea),
-                    ETH_ADDR_ARGS(ea),
-                    IP_ARGS(ip));
+                    ETH_ADDR_ARGS(laddrs->ea),
+                    ETH_ADDR_ARGS(laddrs->ea),
+                    IP_ARGS(laddrs->ipv4_addrs[j].addr));
                 ovn_lflow_add(lflows, op->od, S_SWITCH_IN_L2_LKUP, 150,
                               match, actions);
                 free(match);
                 free(actions);
             }
+
+            if (laddrs->ipv4_addrs) {
+                free(laddrs->ipv4_addrs);
+            }
+            if (laddrs->ipv6_addrs) {
+                free(laddrs->ipv6_addrs);
+            }
+            free(laddrs);
         }
     }
 
@@ -1541,12 +1640,13 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
             /* XXX ARP for neighboring router */
         } else if (op->od->n_router_ports) {
             for (size_t i = 0; i < op->nbs->n_addresses; i++) {
-                struct eth_addr ea;
-                ovs_be32 ip;
-
-                if (ovs_scan(op->nbs->addresses[i],
-                             ETH_ADDR_SCAN_FMT" "IP_SCAN_FMT,
-                             ETH_ADDR_SCAN_ARGS(ea), IP_SCAN_ARGS(&ip))) {
+                struct lport_addresses *laddrs = extract_lport_addresses(
+                    op->nbs->addresses[i]);
+                if (!laddrs) {
+                    continue;
+                }
+                for (size_t k = 0; k < laddrs->n_ipv4_addrs; k++) {
+                    ovs_be32 ip = laddrs->ipv4_addrs[k].addr;
                     for (size_t j = 0; j < op->od->n_router_ports; j++) {
                         /* Get the Logical_Router_Port that the Logical_Port is
                          * connected to, as 'peer'. */
@@ -1574,7 +1674,7 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                                                   "outport = %s; "
                                                   "output;",
                                                   ETH_ADDR_ARGS(peer->mac),
-                                                  ETH_ADDR_ARGS(ea),
+                                                  ETH_ADDR_ARGS(laddrs->ea),
                                                   peer->json_key);
                         ovn_lflow_add(lflows, peer->od,
                                       S_ROUTER_IN_ARP, 200, match, actions);
@@ -1583,6 +1683,14 @@  build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
                         break;
                     }
                 }
+
+                if (laddrs->ipv4_addrs) {
+                    free(laddrs->ipv4_addrs);
+                }
+                if (laddrs->ipv6_addrs) {
+                    free(laddrs->ipv6_addrs);
+                }
+                free(laddrs);
             }
         }
     }
diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
index 4e414ce..b4768d0 100644
--- a/ovn/ovn-nb.xml
+++ b/ovn/ovn-nb.xml
@@ -254,12 +254,12 @@ 
         </p>
 
         <dl>
-          <dt><code><var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var></code></dt>
+          <dt><code>Ethernet address followed by zero or more IPv4 or IPv6 addresses (or both)</code></dt>
           <dd>
             <p>
-              An Ethernet address owned by the logical port.  Like a physical
-              Ethernet NIC, a logical port ordinarily has a single fixed
-              Ethernet address.
+              An Ethernet address defined is owned by the logical port.
+              Like a physical Ethernet NIC, a logical port ordinarily has
+              a single fixed Ethernet address.
             </p>
 
             <p>
@@ -269,27 +269,55 @@ 
               if a MAC learning process had learned that MAC address on the
               port.
             </p>
-          </dd>
 
-          <dt><code><var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var>:<var>xx</var> <var>a</var>.<var>b</var>.<var>c</var>.<var>d</var></code></dt>
-          <dd>
             <p>
-              This form has all the effects of the previous form.  It also
-              indicates that the logical port owns the given IPv4 address.
+              If IPv4 or IPv6 address(es) (or both) are defined, it indicates
+              that the logical port owns the given IP addresses.
             </p>
 
             <p>
-              The OVN logical switch uses this information to synthesize
-              responses to ARP requests without traversing the physical
-              network.  The OVN logical router connected to the logical switch,
-              if any, uses this information to avoid issuing ARP requests for
-              logical switch ports.
+              If IPv4 address(es) are defined, the OVN logical switch uses this
+              information to synthesize responses to ARP requests without
+              traversing the physical network. The OVN logical router connected
+              to the logical switch, if any, uses this information to avoid
+              issuing ARP requests for logical switch ports.
             </p>
 
             <p>
               Note that the order here is important. The Ethernet address must
-              be listed before the IP address.
+              be listed before the IP address(es) if defined. If both IPv4 and
+              IPv6 addresses are defined, then IPv4 addresses should be listed
+              before IPv6 address(es).
+            </p>
+
+            <p>
+              Examples:
             </p>
+
+            <dl>
+              <dt><code>80:fa:5b:06:72:b7</code></dt>
+              <dd>
+                This indicates that the logical port owns the above mac address.
+              </dd>
+
+              <dt><code>80:fa:5b:06:72:b7 10.0.0.4 20.0.0.4</code></dt>
+              <dd>
+                This indicates that the logical port owns the mac address and two
+                IPv4 addresses.
+              </dd>
+
+              <dt><code>80:fa:5b:06:72:b7 fdaa:15f2:72cf:0:f816:3eff:fe20:3f41</code></dt>
+              <dd>
+                This indicates that the logical port owns the mac address and
+                1IPv6 address.
+              </dd>
+
+              <dt><code>80:fa:5b:06:72:b7 10.0.0.4 fdaa:15f2:72cf:0:f816:3eff:fe20:3f41</code></dt>
+              <dd>
+                This indicates that the logical port owns the mac address and
+                1 IPv4 address and 1 IPv6 address.
+              </dd>
+            </dl>
           </dd>
 
           <dt><code>unknown</code></dt>