Message-ID: <76ef3c62584d75beafcd494dc945fe7409409978.camel@perches.com>
Date:   Mon, 27 Aug 2018 09:54:38 -0700
From:   Joe Perches <joe@...ches.com>
To:     rpjday@...shcourse.ca, David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in
 net-sysfs.c?

On Mon, 2018-08-27 at 12:16 -0400, rpjday@...shcourse.ca wrote:
> Quoting David Miller <davem@...emloft.net>:
> 
> > From: "Robert P. J. Day" <rpjday@...shcourse.ca>
> > Date: Mon, 27 Aug 2018 04:55:29 -0400 (EDT)
> > 
> > >   another pedantic oddity -- is there a reason for these two double
> > > negations in net/core/net-sysfs.c?
> > 
> > It turns an arbitrary integer into a boolean, this is a common
> > construct across the kernel tree so I'm surprised you've never seen
> > it before.
> > 
> > Although, I don't know how much more hand holding we're willing to
> > tolerate continuing to give to you at this point.
> > 
> > Thanks.
> 
>    I mentioned in my earlier email that I know what that construct is
> *typically* used for; I also pointed out that, AFAICT, it was totally
> unnecessary in the context of the two routines I mentioned, which
> would appear to ever return only one of two boolean values, 0 or 1.

Both are bool functions, so it's guaranteed and the
!! uses are redundant.

include/linux/netdevice.h:static inline bool netif_carrier_ok(const struct net_device *dev)
include/linux/netdevice.h:static inline bool netif_dormant(const struct net_device *dev)

And there are 4 uses with !!

$ git grep -P  '\!\s*\!\s*netif_(?:dormant|carrier_ok)\s*\('
drivers/net/team/team.c:        __team_port_change_port_added(port, !!netif_carrier_ok(port_dev));
drivers/net/usb/r8152.c:        bool sw_linking = !!netif_carrier_ok(tp->netdev);
net/core/net-sysfs.c:           return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
net/core/net-sysfs.c:           return sprintf(buf, fmt_dec, !!netif_dormant(netdev));

out of the ~400 or so treewide.

Only redeeming use of the !! is the fmt_dec expects
an int and this forces the type to int, though the
compiler would do that automatically anyway.

I'd suggest removing the fmt_<foo> uses for clarity.
---
 net/core/net-sysfs.c | 49 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index bd67c4d0fcfd..ecaa684f4b92 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -31,11 +31,6 @@
 #include "net-sysfs.h"
 
 #ifdef CONFIG_SYSFS
-static const char fmt_hex[] = "%#x\n";
-static const char fmt_dec[] = "%d\n";
-static const char fmt_ulong[] = "%lu\n";
-static const char fmt_u64[] = "%llu\n";
-
 static inline int dev_isalive(const struct net_device *dev)
 {
 	return dev->reg_state <= NETREG_REGISTERED;
@@ -107,26 +102,26 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
 	return ret;
 }
 
-NETDEVICE_SHOW_RO(dev_id, fmt_hex);
-NETDEVICE_SHOW_RO(dev_port, fmt_dec);
-NETDEVICE_SHOW_RO(addr_assign_type, fmt_dec);
-NETDEVICE_SHOW_RO(addr_len, fmt_dec);
-NETDEVICE_SHOW_RO(ifindex, fmt_dec);
-NETDEVICE_SHOW_RO(type, fmt_dec);
-NETDEVICE_SHOW_RO(link_mode, fmt_dec);
+NETDEVICE_SHOW_RO(dev_id, "%#x\n");
+NETDEVICE_SHOW_RO(dev_port, "%d\n");
+NETDEVICE_SHOW_RO(addr_assign_type, "%d\n");
+NETDEVICE_SHOW_RO(addr_len, "%d\n");
+NETDEVICE_SHOW_RO(ifindex, "%d\n");
+NETDEVICE_SHOW_RO(type, "%d\n");
+NETDEVICE_SHOW_RO(link_mode, "%d\n");
 
 static ssize_t iflink_show(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
 	struct net_device *ndev = to_net_dev(dev);
 
-	return sprintf(buf, fmt_dec, dev_get_iflink(ndev));
+	return sprintf(buf, "%d\n", dev_get_iflink(ndev));
 }
 static DEVICE_ATTR_RO(iflink);
 
 static ssize_t format_name_assign_type(const struct net_device *dev, char *buf)
 {
-	return sprintf(buf, fmt_dec, dev->name_assign_type);
+	return sprintf(buf, "%d\n", dev->name_assign_type);
 }
 
 static ssize_t name_assign_type_show(struct device *dev,
@@ -188,7 +183,7 @@ static ssize_t carrier_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 
 	if (netif_running(netdev))
-		return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
+		return sprintf(buf, "%d\n", !!netif_carrier_ok(netdev));
 
 	return -EINVAL;
 }
@@ -207,7 +202,7 @@ static ssize_t speed_show(struct device *dev,
 		struct ethtool_link_ksettings cmd;
 
 		if (!__ethtool_get_link_ksettings(netdev, &cmd))
-			ret = sprintf(buf, fmt_dec, cmd.base.speed);
+			ret = sprintf(buf, "%d\n", cmd.base.speed);
 	}
 	rtnl_unlock();
 	return ret;
@@ -254,7 +249,7 @@ static ssize_t dormant_show(struct device *dev,
 	struct net_device *netdev = to_net_dev(dev);
 
 	if (netif_running(netdev))
-		return sprintf(buf, fmt_dec, !!netif_dormant(netdev));
+		return sprintf(buf, "%d\n", !!netif_dormant(netdev));
 
 	return -EINVAL;
 }
@@ -295,7 +290,7 @@ static ssize_t carrier_changes_show(struct device *dev,
 {
 	struct net_device *netdev = to_net_dev(dev);
 
-	return sprintf(buf, fmt_dec,
+	return sprintf(buf, "%d\n",
 		       atomic_read(&netdev->carrier_up_count) +
 		       atomic_read(&netdev->carrier_down_count));
 }
@@ -307,7 +302,7 @@ static ssize_t carrier_up_count_show(struct device *dev,
 {
 	struct net_device *netdev = to_net_dev(dev);
 
-	return sprintf(buf, fmt_dec, atomic_read(&netdev->carrier_up_count));
+	return sprintf(buf, "%d\n", atomic_read(&netdev->carrier_up_count));
 }
 static DEVICE_ATTR_RO(carrier_up_count);
 
@@ -317,7 +312,7 @@ static ssize_t carrier_down_count_show(struct device *dev,
 {
 	struct net_device *netdev = to_net_dev(dev);
 
-	return sprintf(buf, fmt_dec, atomic_read(&netdev->carrier_down_count));
+	return sprintf(buf, "%d\n", atomic_read(&netdev->carrier_down_count));
 }
 static DEVICE_ATTR_RO(carrier_down_count);
 
@@ -333,7 +328,7 @@ static ssize_t mtu_store(struct device *dev, struct device_attribute *attr,
 {
 	return netdev_store(dev, attr, buf, len, change_mtu);
 }
-NETDEVICE_SHOW_RW(mtu, fmt_dec);
+NETDEVICE_SHOW_RW(mtu, "%d\n");
 
 static int change_flags(struct net_device *dev, unsigned long new_flags)
 {
@@ -345,7 +340,7 @@ static ssize_t flags_store(struct device *dev, struct device_attribute *attr,
 {
 	return netdev_store(dev, attr, buf, len, change_flags);
 }
-NETDEVICE_SHOW_RW(flags, fmt_hex);
+NETDEVICE_SHOW_RW(flags, "%#x\n");
 
 static ssize_t tx_queue_len_store(struct device *dev,
 				  struct device_attribute *attr,
@@ -356,7 +351,7 @@ static ssize_t tx_queue_len_store(struct device *dev,
 
 	return netdev_store(dev, attr, buf, len, dev_change_tx_queue_len);
 }
-NETDEVICE_SHOW_RW(tx_queue_len, fmt_dec);
+NETDEVICE_SHOW_RW(tx_queue_len, "%d\n");
 
 static int change_gro_flush_timeout(struct net_device *dev, unsigned long val)
 {
@@ -373,7 +368,7 @@ static ssize_t gro_flush_timeout_store(struct device *dev,
 
 	return netdev_store(dev, attr, buf, len, change_gro_flush_timeout);
 }
-NETDEVICE_SHOW_RW(gro_flush_timeout, fmt_ulong);
+NETDEVICE_SHOW_RW(gro_flush_timeout, "%lu\n");
 
 static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,
 			     const char *buf, size_t len)
@@ -431,7 +426,7 @@ static ssize_t group_store(struct device *dev, struct device_attribute *attr,
 {
 	return netdev_store(dev, attr, buf, len, change_group);
 }
-NETDEVICE_SHOW(group, fmt_dec);
+NETDEVICE_SHOW(group, "%d\n");
 static DEVICE_ATTR(netdev_group, 0644, group_show, group_store);
 
 static int change_proto_down(struct net_device *dev, unsigned long proto_down)
@@ -445,7 +440,7 @@ static ssize_t proto_down_store(struct device *dev,
 {
 	return netdev_store(dev, attr, buf, len, change_proto_down);
 }
-NETDEVICE_SHOW_RW(proto_down, fmt_dec);
+NETDEVICE_SHOW_RW(proto_down, "%d\n");
 
 static ssize_t phys_port_id_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
@@ -568,7 +563,7 @@ static ssize_t netstat_show(const struct device *d,
 		struct rtnl_link_stats64 temp;
 		const struct rtnl_link_stats64 *stats = dev_get_stats(dev, &temp);
 
-		ret = sprintf(buf, fmt_u64, *(u64 *)(((u8 *)stats) + offset));
+		ret = sprintf(buf, "%llu\n", *(u64 *)(((u8 *)stats) + offset));
 	}
 	read_unlock(&dev_base_lock);
 	return ret;

Powered by blists - more mailing lists