[ovs-dev] [PATCH 04/15] netdev-linux: Avoid fiddling with indeterminate data.

[Ben Pfaff]

On Thu, Feb 11, 2010 at 11:43:41AM -0500, Jesse Gross wrote:
> On Wed, Feb 10, 2010 at 2:30 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> >
> > @@ -1255,9 +1253,10 @@ netdev_linux_get_stats(const struct netdev *netdev_,
> >         int ifindex;
> >
> >         error = get_ifindex(netdev_, &ifindex);
> > -        if (!error) {
> > -            error = get_stats_via_netlink(ifindex, collect_stats);
> > +        if (error) {
> > +            return error;
> >         }
> > +        error = get_stats_via_netlink(ifindex, collect_stats);
> >     } else {
> >         error = get_stats_via_proc(netdev_get_name(netdev_),
> > collect_stats);
> >     }
> 
> Shouldn't we also return immediately if either of the get stats functions
> fails?

You're right.

I updated the patch to:

commit b62aeed2ab06ecb9a3b3a82dc42ebfc2703ef52f
Author: Ben Pfaff <blp at nicira.com>
Date:   Thu Feb 11 10:34:45 2010 -0800

    netdev-linux: Avoid fiddling with indeterminate data.
    
    If we are using netlink to get stats and get_ifindex() fails, then for
    an internal network device we will then swap around a bunch of
    indeterminate (uninitialized) data values.  That won't hurt anything--the
    caller will still set them to all-1-bits due to the error--but it still
    seems wrong.  So this commit avoid it.
    
    Found using Clang (http://clang-analyzer.llvm.org/).

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index ccc3f84..6d46b09 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -1223,9 +1223,7 @@ netdev_linux_get_stats(const struct netdev *netdev_,
     COVERAGE_INC(netdev_get_stats);
 
     if (!(netdev_dev->cache_valid & VALID_IS_INTERNAL)) {
-        netdev_dev->is_internal = !strcmp(netdev_get_type(netdev_),
-                                                "tap");
-
+        netdev_dev->is_internal = !strcmp(netdev_get_type(netdev_), "tap");
         if (!netdev_dev->is_internal) {
             struct ethtool_drvinfo drvinfo;
 
@@ -1266,7 +1264,7 @@ netdev_linux_get_stats(const struct netdev *netdev_,
      * will appear to be swapped relative to the other ports since we are the
      * one sending the data, not a remote computer.  For consistency, we swap
      * them back here. */
-    if (netdev_dev->is_internal) {
+    if (!error && netdev_dev->is_internal) {
         stats->rx_packets = raw_stats.tx_packets;
         stats->tx_packets = raw_stats.rx_packets;
         stats->rx_bytes = raw_stats.tx_bytes;