[ovs-dev] [PATCH 2/2] socket-util: Use correct address family in set_dscp(), instead of guessing.

Alex Wang alexw at nicira.com
Fri Feb 20 19:15:24 UTC 2015


Tested on Freebsd 9.3~ works fine, with the following addition:

Acked-by: Alex Wang <alexw at nicira.com>
Thx a lot for fixing~~~


diff --git a/python/ovs/socket_util.py b/python/ovs/socket_util.py
index 1af6474..416784e 100644
--- a/python/ovs/socket_util.py
+++ b/python/ovs/socket_util.py
@@ -213,14 +213,16 @@ def inet_open_active(style, target, default_port,
dscp):
         is_addr_inet = is_valid_ipv4_address(address[0])
         if is_addr_inet:
             sock = socket.socket(socket.AF_INET, style, 0)
+            family = socket.AF_INET
         else:
             sock = socket.socket(socket.AF_INET6, style, 0)
+            family = socket.AF_INET6
     except socket.error, e:
         return get_exception_errno(e), None

     try:
         set_nonblocking(sock)
-        set_dscp(sock, dscp)
+        set_dscp(sock, family, dscp)
         try:
             sock.connect(address)
         except socket.error, e:
@@ -292,21 +294,20 @@ def set_nonblocking(sock):
                  % os.strerror(get_exception_errno(e)))


-def set_dscp(sock, dscp):
+def set_dscp(sock, family, dscp):
     if dscp > 63:
         raise ValueError("Invalid dscp %d" % dscp)

-    # Note: this function is used for both of IPv4 and IPv6 sockets
-    success = False
     val = dscp << 2
-    try:
-        sock.setsockopt(socket.IPPROTO_IP, socket.IP_TOS, val)
-    except socket.error, e:
-        if get_exception_errno(e) != errno.ENOPROTOOPT:
+    if family == socket.AF_INET:
+        try:
+            sock.setsockopt(socket.IPPROTO_IP, socket.IP_TOS, val)
+        except socket.error, e:
             raise
-    success = True
-    try:
-        sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_TCLASS, val)
-    except socket.error, e:
-        if get_exception_errno(e) != errno.ENOPROTOOPT or not success:
+    elif family == socket.AF_INET6:
+        try:
+            sock.setsockopt(socket.IPPROTO_IPV6, socket.IPV6_TCLASS, val)
+        except socket.error, e:
             raise
+    else:
+        raise


On Fri, Feb 20, 2015 at 9:55 AM, Alex Wang <alexw at nicira.com> wrote:

>
> Bug reported by: Atanu Ghosh <atanu at acm.org>
> I'll review + test this series soon~
>
>
> On Fri, Feb 20, 2015 at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:
>
>> The set_dscp() function, until now, tried to set the DSCP as IPv4 and as
>> IPv6. This worked OK on Linux, where an ENOPROTOOPT error made it really
>> clear which one was wrong, but FreeBSD uses EINVAL instead, which has
>> multiple meanings and which it therefore seems somewhat risky to ignore.
>> Instead, this commit just tries to set the correct address family's DSCP
>> option.
>>
>> Signed-off-by: Ben Pfaff <blp at nicira.com>
>> ---
>> I'd like to credit whoever reported this bug, but I don't remember who it
>> is.  Can someone remind me?
>>
>>  lib/socket-util.c | 44 ++++++++++++++++++++------------------------
>>  lib/socket-util.h |  4 ++--
>>  2 files changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/lib/socket-util.c b/lib/socket-util.c
>> index 8949da7..206e17b 100644
>> --- a/lib/socket-util.c
>> +++ b/lib/socket-util.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira,
>> Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -101,11 +101,14 @@ setsockopt_tcp_nodelay(int fd)
>>      }
>>  }
>>
>> +/* Sets the DSCP value of socket 'fd' to 'dscp', which must be 63 or
>> less.
>> + * 'family' must indicate the socket's address family (AF_INET or
>> AF_INET6, to
>> + * do anything useful). */
>>  int
>> -set_dscp(int fd, uint8_t dscp)
>> +set_dscp(int fd, int family, uint8_t dscp)
>>  {
>> +    int retval;
>>      int val;
>> -    bool success;
>>
>>  #ifdef _WIN32
>>      /* XXX: Consider using QoS2 APIs for Windows to set dscp. */
>> @@ -115,29 +118,22 @@ set_dscp(int fd, uint8_t dscp)
>>      if (dscp > 63) {
>>          return EINVAL;
>>      }
>> -
>> -    /* Note: this function is used for both of IPv4 and IPv6 sockets */
>> -    success = false;
>>      val = dscp << 2;
>> -    if (setsockopt(fd, IPPROTO_IP, IP_TOS, &val, sizeof val)) {
>> -        if (sock_errno() != ENOPROTOOPT) {
>> -            return sock_errno();
>> -        }
>> -    } else {
>> -        success = true;
>> -    }
>> -    if (setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &val, sizeof val)) {
>> -        if (sock_errno() != ENOPROTOOPT) {
>> -            return sock_errno();
>> -        }
>> -    } else {
>> -        success = true;
>> -    }
>> -    if (!success) {
>> +
>> +    switch (family) {
>> +    case AF_INET:
>> +        retval = setsockopt(fd, IPPROTO_IP, IP_TOS, &val, sizeof val);
>> +        break;
>> +
>> +    case AF_INET6:
>> +        retval = setsockopt(fd, IPPROTO_IPV6, IPV6_TCLASS, &val, sizeof
>> val);
>> +        break;
>> +
>> +    default:
>>          return ENOPROTOOPT;
>>      }
>>
>> -    return 0;
>> +    return retval ? sock_errno() : 0;
>>  }
>>
>>  /* Translates 'host_name', which must be a string representation of an IP
>> @@ -470,7 +466,7 @@ inet_open_active(int style, const char *target,
>> uint16_t default_port,
>>      /* The dscp bits must be configured before connect() to ensure that
>> the
>>       * TOS field is set during the connection establishment.  If set
>> after
>>       * connect(), the handshake SYN frames will be sent with a TOS of 0.
>> */
>> -    error = set_dscp(fd, dscp);
>> +    error = set_dscp(fd, ss.ss_family, dscp);
>>      if (error) {
>>          VLOG_ERR("%s: set_dscp: %s", target, sock_strerror(error));
>>          goto exit;
>> @@ -611,7 +607,7 @@ inet_open_passive(int style, const char *target, int
>> default_port,
>>      /* The dscp bits must be configured before connect() to ensure that
>> the TOS
>>       * field is set during the connection establishment.  If set after
>>       * connect(), the handshake SYN frames will be sent with a TOS of 0.
>> */
>> -    error = set_dscp(fd, dscp);
>> +    error = set_dscp(fd, ss.ss_family, dscp);
>>      if (error) {
>>          VLOG_ERR("%s: set_dscp: %s", target, sock_strerror(error));
>>          goto error;
>> diff --git a/lib/socket-util.h b/lib/socket-util.h
>> index 5b94f20..1178fb8 100644
>> --- a/lib/socket-util.h
>> +++ b/lib/socket-util.h
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira,
>> Inc.
>>   *
>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>   * you may not use this file except in compliance with the License.
>> @@ -30,7 +30,7 @@
>>  int set_nonblocking(int fd);
>>  void xset_nonblocking(int fd);
>>  void setsockopt_tcp_nodelay(int fd);
>> -int set_dscp(int fd, uint8_t dscp);
>> +int set_dscp(int fd, int family, uint8_t dscp);
>>
>>  int lookup_ip(const char *host_name, struct in_addr *address);
>>  int lookup_ipv6(const char *host_name, struct in6_addr *address);
>> --
>> 2.1.3
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
>



More information about the dev mailing list