diff mbox series

[ovs-dev,v2] util: Avoid double parsing of LB vip and backend ip

Message ID 20221019054247.648938-1-amusil@redhat.com
State Accepted
Headers show
Series [ovs-dev,v2] util: Avoid double parsing of LB vip and backend ip | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ales Musil Oct. 19, 2022, 5:42 a.m. UTC 
The current code was parsing the ip twice, once for
the "inet_parse_active" and second time by calling
either "ip_parse" or "ipv6_parse". Avoid that by
getting the "in6_addr" directly from "sockaddr_storage".

Acked-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Ales Musil <amusil@redhat.com>
---
v2: Rebase on top of current main
    Address comment from Numan
---
 lib/lb.c              | 20 +++-----------------
 lib/ovn-util.c        | 11 +++++++----
 lib/ovn-util.h        |  3 ++-
 utilities/ovn-trace.c | 20 ++++++--------------
 4 files changed, 18 insertions(+), 36 deletions(-)

Comments

Dumitru Ceara Oct. 20, 2022, 2:30 p.m. UTC | #1 
On 10/19/22 07:42, Ales Musil wrote:
> The current code was parsing the ip twice, once for
> the "inet_parse_active" and second time by calling
> either "ip_parse" or "ipv6_parse". Avoid that by
> getting the "in6_addr" directly from "sockaddr_storage".
> 
> Acked-by: Numan Siddique <numans@ovn.org>
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
> v2: Rebase on top of current main
>     Address comment from Numan
> ---

Thanks Ales and Numan!  I pushed this to the main branch with the following
minor change to initialize output arguments in the same order on both
branches:

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index a89af6f28d..5dca727146 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -797,9 +797,9 @@ ip_address_and_port_from_lb_key(const char *key, char **ip_address,
         VLOG_WARN_RL(&rl, "bad ip address or port for load balancer key %s",
                      key);
         *ip_address = NULL;
+        memset(ip, 0, sizeof(*ip));
         *port = 0;
         *addr_family = 0;
-        memset(ip, 0, sizeof(*ip));
         return false;
     }
diff mbox series

Patch

diff --git a/lib/lb.c b/lib/lb.c
index 77674ea28..ab5de38a8 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -78,18 +78,11 @@  bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
     int addr_family;
 
     if (!ip_address_and_port_from_lb_key(lb_key, &lb_vip->vip_str,
-                                         &lb_vip->vip_port, &addr_family)) {
+                                         &lb_vip->vip, &lb_vip->vip_port,
+                                         &addr_family)) {
         return false;
     }
 
-    if (addr_family == AF_INET) {
-        ovs_be32 vip4;
-        ip_parse(lb_vip->vip_str, &vip4);
-        in6_addr_set_mapped_ipv4(&lb_vip->vip, vip4);
-    } else {
-        ipv6_parse(lb_vip->vip_str, &lb_vip->vip);
-    }
-
     /* Format for backend ips: "IP1:port1,IP2:port2,...". */
     size_t n_backends = 0;
     size_t n_allocated_backends = 0;
@@ -108,7 +101,7 @@  bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
         struct ovn_lb_backend *backend = &lb_vip->backends[n_backends];
         int backend_addr_family;
         if (!ip_address_and_port_from_lb_key(token, &backend->ip_str,
-                                             &backend->port,
+                                             &backend->ip, &backend->port,
                                              &backend_addr_family)) {
             continue;
         }
@@ -118,13 +111,6 @@  bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
             continue;
         }
 
-        if (addr_family == AF_INET) {
-            ovs_be32 ip4;
-            ip_parse(backend->ip_str, &ip4);
-            in6_addr_set_mapped_ipv4(&backend->ip, ip4);
-        } else {
-            ipv6_parse(backend->ip_str, &backend->ip);
-        }
         n_backends++;
     }
     free(tokstr);
diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index d80db179a..a89af6f28 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -781,14 +781,15 @@  ovn_smap_get_uint(const struct smap *smap, const char *key, unsigned int def)
     return u_value;
 }
 
-/* For a 'key' of the form "IP:port" or just "IP", sets 'port' and
- * 'ip_address'.  The caller must free() the memory allocated for
- * 'ip_address'.
+/* For a 'key' of the form "IP:port" or just "IP", sets 'port',
+ * 'ip_address' and 'ip' ('struct in6_addr' IPv6 or IPv4 mapped address).
+ * The caller must free() the memory allocated for 'ip_address'.
  * Returns true if parsing of 'key' was successful, false otherwise.
  */
 bool
 ip_address_and_port_from_lb_key(const char *key, char **ip_address,
-                                uint16_t *port, int *addr_family)
+                                struct in6_addr *ip, uint16_t *port,
+                                int *addr_family)
 {
     struct sockaddr_storage ss;
     if (!inet_parse_active(key, 0, &ss, false, NULL)) {
@@ -798,12 +799,14 @@  ip_address_and_port_from_lb_key(const char *key, char **ip_address,
         *ip_address = NULL;
         *port = 0;
         *addr_family = 0;
+        memset(ip, 0, sizeof(*ip));
         return false;
     }
 
     struct ds s = DS_EMPTY_INITIALIZER;
     ss_format_address_nobracks(&ss, &s);
     *ip_address = ds_steal_cstr(&s);
+    *ip = ss_get_address(&ss);
     *port = ss_get_port(&ss);
     *addr_family = ss.ss_family;
     return true;
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 145f974ed..a1f1cf0ad 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -241,7 +241,8 @@  char *str_tolower(const char *orig);
         case OVN_OPT_USER_GROUP:
 
 bool ip_address_and_port_from_lb_key(const char *key, char **ip_address,
-                                     uint16_t *port, int *addr_family);
+                                     struct in6_addr *ip, uint16_t *port,
+                                     int *addr_family);
 
 /* Returns the internal OVN version. The caller must free the returned
  * value. */
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index d9e7129d9..6fa5137d9 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -2601,23 +2601,15 @@  get_lb_vip_data(struct flow *uflow, struct in6_addr *vip,
     SBREC_LOAD_BALANCER_FOR_EACH (sbdb, ovnsb_idl) {
         struct smap_node *node;
         SMAP_FOR_EACH (node, &sbdb->vips) {
-            if (!ip_address_and_port_from_lb_key(node->key, vip_str,
-                                                 port, &family)) {
+            if (!ip_address_and_port_from_lb_key(node->key, vip_str, vip, port,
+                                                 &family)) {
                 continue;
             }
 
-            if (family == AF_INET) {
-                ovs_be32 vip4;
-                ip_parse(*vip_str, &vip4);
-                in6_addr_set_mapped_ipv4(vip, vip4);
-                if (vip4 == uflow->ct_nw_dst) {
-                    return true;
-                }
-            } else {
-                ipv6_parse(*vip_str, vip);
-                if (ipv6_addr_equals(vip, &uflow->ct_ipv6_dst)) {
-                    return true;
-                }
+            if ((family == AF_INET
+                 && in6_addr_get_mapped_ipv4(vip) == uflow->ct_nw_dst)
+                || ipv6_addr_equals(vip, &uflow->ct_ipv6_dst)) {
+                return true;
             }
             free(*vip_str);
         }