any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[Robert P. J. Day]

  another pedantic oddity -- is there a reason for these two double
negations in net/core/net-sysfs.c?

static ssize_t carrier_show(struct device *dev,
                            struct device_attribute *attr, char *buf)
{
        struct net_device *netdev = to_net_dev(dev);

        if (netif_running(netdev))
                return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));

...

static ssize_t dormant_show(struct device *dev,
                            struct device_attribute *attr, char *buf)
{
        struct net_device *netdev = to_net_dev(dev);

        if (netif_running(netdev))
                return sprintf(buf, fmt_dec, !!netif_dormant(netdev));

  i understand the normal rationale for !! in assuring a final boolean
value of precisely either 0 or 1 (here for the sake of printing), but
given that those two routines are declared with a return value of
"bool" in netdevice.h, i don't see any way that they can return
anything *other* than 0 or 1. i realize it can't possibly hurt, but
whenever i see this construct, i normally assume there's a *reason*
for it, but i can't see what it's doing in those two places.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                  http://crashcourse.ca/dokuwiki

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[David Miller]

From: "Robert P. J. Day" <rpjday@crashcourse.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.

Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[rpjday]

Quoting David Miller <davem@davemloft.net>:

> From: "Robert P. J. Day" <rpjday@crashcourse.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.

   My experience with kernel code is that one should not introduce
unnecessary complexities, which suggests (as I stated) that there
seems to be no value in the "!!" construct *in those particular
cases*, hence my curiosity.

   If you want to criticize my question, at least have the courtesy
to represent it accurately.

rday

Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[Eric Dumazet]

On 08/27/2018 09:16 AM, rpjday@crashcourse.ca wrote:
> 
> Quoting David Miller <davem@davemloft.net>:
> 
>> From: "Robert P. J. Day" <rpjday@crashcourse.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.
> 
>   My experience with kernel code is that one should not introduce
> unnecessary complexities, which suggests (as I stated) that there
> seems to be no value in the "!!" construct *in those particular
> cases*, hence my curiosity.

Have you checked git history ?

My guess is that netif_carrier_ok() used to return an int, not a bool.

!!netif_carrier_ok() was not complexity, it was probably shorter than
the form used in 

u32 ethtool_op_get_link(struct net_device *dev)
{
	return netif_carrier_ok(dev) ? 1 : 0;
}

But really, this is really trivial stuff, we have more interesting stuff
to take care of these days.

Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[rpjday]

Quoting Eric Dumazet <eric.dumazet@gmail.com>:

> On 08/27/2018 09:16 AM, rpjday@crashcourse.ca wrote:
>>
>> Quoting David Miller <davem@davemloft.net>:
>>
>>> From: "Robert P. J. Day" <rpjday@crashcourse.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.
>>
>>   My experience with kernel code is that one should not introduce
>> unnecessary complexities, which suggests (as I stated) that there
>> seems to be no value in the "!!" construct *in those particular
>> cases*, hence my curiosity.
>
> Have you checked git history ?
>
> My guess is that netif_carrier_ok() used to return an int, not a bool.
>
> !!netif_carrier_ok() was not complexity, it was probably shorter than
> the form used in
>
> u32 ethtool_op_get_link(struct net_device *dev)
> {
> 	return netif_carrier_ok(dev) ? 1 : 0;
> }
>
> But really, this is really trivial stuff, we have more interesting stuff
> to take care of these days.

   Point taken -- for trivialities like this, I'll check Git history, and
apologies for the noise on that one.

rday

Re: any reason for "!!netif_carrier_ok" and "!!netif_dormant" in net-sysfs.c?

[Joe Perches]

On Mon, 2018-08-27 at 12:16 -0400, rpjday@crashcourse.ca wrote:
> Quoting David Miller <davem@davemloft.net>:
> 
> > From: "Robert P. J. Day" <rpjday@crashcourse.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;