Hi,
Here's a patch that adds a new sysfs attribute for net devices. The new
attribute 'carrier' exposes the result of netif_carrier_ok() so that when
a network device has carrier the attribute value is 1 and when there is no
carrier the attribute value is 0.
Very rellevant attribute for network devices in my oppinion, and sysfs is
the logical place for it.
I've tested this only on my own machine, but I get the expected results:
With network cable plugged into eth0 :
juhl@dragon:~$ cat /sys/class/net/eth0/carrier
1
juhl@dragon:~$
With network cable unplugged :
juhl@dragon:~$ cat /sys/class/net/eth0/carrier
0
juhl@dragon:~$
Please review and consider applying.
Signed-off-by: Jesper Juhl <[email protected]>
diff -u linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk5/net/core/net-sysfs.c
--- linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c 2004-09-14 23:19:53.000000000 +0200
+++ linux-2.6.9-rc2-bk5/net/core/net-sysfs.c 2004-09-27 00:24:01.000000000 +0200
@@ -126,8 +126,21 @@
return -EINVAL;
}
+static ssize_t show_carrier(struct class_device *dev, char *buf)
+{
+ struct net_device *net = to_net_dev(dev);
+ if (netif_running(net)) {
+ if (netif_carrier_ok(net))
+ return snprintf(buf, 3, "%d\n", 1);
+ else
+ return snprintf(buf, 3, "%d\n", 0);
+ }
+ return -EINVAL;
+}
+
static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
+static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
/* read-write attributes */
NETDEVICE_SHOW(mtu, fmt_dec);
@@ -186,6 +199,7 @@
&class_device_attr_type,
&class_device_attr_address,
&class_device_attr_broadcast,
+ &class_device_attr_carrier,
NULL
};
On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> Hi,
>
> Here's a patch that adds a new sysfs attribute for net devices. The new
> attribute 'carrier' exposes the result of netif_carrier_ok() so that when
> a network device has carrier the attribute value is 1 and when there is no
> carrier the attribute value is 0.
> Very rellevant attribute for network devices in my oppinion, and sysfs is
> the logical place for it.
>
> I've tested this only on my own machine, but I get the expected results:
>
> With network cable plugged into eth0 :
>
> juhl@dragon:~$ cat /sys/class/net/eth0/carrier
> 1
> juhl@dragon:~$
>
> With network cable unplugged :
>
> juhl@dragon:~$ cat /sys/class/net/eth0/carrier
> 0
> juhl@dragon:~$
>
> Please review and consider applying.
>
> Signed-off-by: Jesper Juhl <[email protected]>
>
> diff -u linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk5/net/core/net-sysfs.c
> --- linux-2.6.9-rc2-bk5-orig/net/core/net-sysfs.c 2004-09-14 23:19:53.000000000 +0200
> +++ linux-2.6.9-rc2-bk5/net/core/net-sysfs.c 2004-09-27 00:24:01.000000000 +0200
> @@ -126,8 +126,21 @@
> return -EINVAL;
> }
>
> +static ssize_t show_carrier(struct class_device *dev, char *buf)
> +{
> + struct net_device *net = to_net_dev(dev);
> + if (netif_running(net)) {
> + if (netif_carrier_ok(net))
> + return snprintf(buf, 3, "%d\n", 1);
> + else
> + return snprintf(buf, 3, "%d\n", 0);
Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
The most concise format of this would be:
return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> + }
> + return -EINVAL;
> +}
> +
> static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
> static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
> +static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
>
> /* read-write attributes */
> NETDEVICE_SHOW(mtu, fmt_dec);
> @@ -186,6 +199,7 @@
> &class_device_attr_type,
> &class_device_attr_address,
> &class_device_attr_broadcast,
> + &class_device_attr_carrier,
> NULL
> };
>
On Mon, 27 Sep 2004, Stephen Hemminger wrote:
> Date: Mon, 27 Sep 2004 10:29:13 -0700
> From: Stephen Hemminger <[email protected]>
> To: Jesper Juhl <[email protected]>
> Cc: linux-kernel <[email protected]>,
> Nico Schottelius <[email protected]>
> Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
>
> On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> >
> > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > +{
> > + struct net_device *net = to_net_dev(dev);
> > + if (netif_running(net)) {
> > + if (netif_carrier_ok(net))
> > + return snprintf(buf, 3, "%d\n", 1);
> > + else
> > + return snprintf(buf, 3, "%d\n", 0);
>
> Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> The most concise format of this would be:
> return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
>
I see your point and I'll create a new patch this evening when I get home
from work.
Thank you for your feedback.
--
Jesper Juhl
Stephen Hemminger wrote:
> ....
>>+{
>>+ struct net_device *net = to_net_dev(dev);
>>+ if (netif_running(net)) {
>>+ if (netif_carrier_ok(net))
>>+ return snprintf(buf, 3, "%d\n", 1);
>>+ else
>>+ return snprintf(buf, 3, "%d\n", 0);
>
>
> Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> The most concise format of this would be:
> return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
<nitpick>
Since netif_carrier_ok already has a "!" in it, it is guaranteed to
return a 0 / 1 result. so this could be:
return sprintf(buf, dec_fmt, netif_carrier_ok(dev));
Of course your way is more robust to future 'netif_carrier_ok' changes
and the compiler should optimize it way anyway since it is an inline
function, so I actually prefer the !! version :)
</nitpick>
--
Paulo Marques - http://www.grupopie.com
To err is human, but to really foul things up requires a computer.
Farmers' Almanac, 1978
hi :)
On Tue, Sep 28, 2004 at 01:10:21PM +0100, Paulo Marques wrote:
> Of course your way is more robust to future 'netif_carrier_ok' changes
> and the compiler should optimize it way anyway since it is an inline
> function, so I actually prefer the !! version :)
But perhaps those future changes would be interesting for userspace,
too?
--
Martin Waitz
On Tue, 28 Sep 2004, Jesper Juhl wrote:
> On Mon, 27 Sep 2004, Stephen Hemminger wrote:
>
> > Date: Mon, 27 Sep 2004 10:29:13 -0700
> > From: Stephen Hemminger <[email protected]>
> > To: Jesper Juhl <[email protected]>
> > Cc: linux-kernel <[email protected]>,
> > Nico Schottelius <[email protected]>
> > Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
> >
> > On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> > >
> > > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > > +{
> > > + struct net_device *net = to_net_dev(dev);
> > > + if (netif_running(net)) {
> > > + if (netif_carrier_ok(net))
> > > + return snprintf(buf, 3, "%d\n", 1);
> > > + else
> > > + return snprintf(buf, 3, "%d\n", 0);
> >
> > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > The most concise format of this would be:
> > return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> >
>
> I see your point and I'll create a new patch this evening when I get home
> from work.
> Thank you for your feedback.
>
Here's a second try at a patch to properly implement a 'carrier' sysfs
attribute for net devices.
Signed-off-by: Jesper Juhl <[email protected]>
diff -u linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk14/net/core/net-sysfs.c
--- linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c 2004-09-14 23:19:53.000000000 +0200
+++ linux-2.6.9-rc2-bk14/net/core/net-sysfs.c 2004-09-28 19:37:45.000000000 +0200
@@ -126,8 +126,18 @@
return -EINVAL;
}
+static ssize_t show_carrier(struct class_device *dev, char *buf)
+{
+ struct net_device *netdev = to_net_dev(dev);
+ if (netif_running(netdev)) {
+ return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
+ }
+ return -EINVAL;
+}
+
static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
+static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
/* read-write attributes */
NETDEVICE_SHOW(mtu, fmt_dec);
@@ -186,6 +196,7 @@
&class_device_attr_type,
&class_device_attr_address,
&class_device_attr_broadcast,
+ &class_device_attr_carrier,
NULL
};
On Tue, 2004-09-28 at 21:23 +0200, Jesper Juhl wrote:
> On Tue, 28 Sep 2004, Jesper Juhl wrote:
>
> > On Mon, 27 Sep 2004, Stephen Hemminger wrote:
> >
> > > Date: Mon, 27 Sep 2004 10:29:13 -0700
> > > From: Stephen Hemminger <[email protected]>
> > > To: Jesper Juhl <[email protected]>
> > > Cc: linux-kernel <[email protected]>,
> > > Nico Schottelius <[email protected]>
> > > Subject: Re: [PATCH] add sysfs attribute 'carrier' for net devices
> > >
> > > On Mon, 2004-09-27 at 00:51 +0200, Jesper Juhl wrote:
> > > >
> > > > +static ssize_t show_carrier(struct class_device *dev, char *buf)
> > > > +{
> > > > + struct net_device *net = to_net_dev(dev);
> > > > + if (netif_running(net)) {
> > > > + if (netif_carrier_ok(net))
> > > > + return snprintf(buf, 3, "%d\n", 1);
> > > > + else
> > > > + return snprintf(buf, 3, "%d\n", 0);
> > >
> > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > The most concise format of this would be:
> > > return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> > >
> >
> > I see your point and I'll create a new patch this evening when I get home
> > from work.
> > Thank you for your feedback.
> >
>
> Here's a second try at a patch to properly implement a 'carrier' sysfs
> attribute for net devices.
>
> Signed-off-by: Jesper Juhl <[email protected]>
>
> diff -u linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c linux-2.6.9-rc2-bk14/net/core/net-sysfs.c
> --- linux-2.6.9-rc2-bk14-orig/net/core/net-sysfs.c 2004-09-14 23:19:53.000000000 +0200
> +++ linux-2.6.9-rc2-bk14/net/core/net-sysfs.c 2004-09-28 19:37:45.000000000 +0200
> @@ -126,8 +126,18 @@
> return -EINVAL;
> }
>
> +static ssize_t show_carrier(struct class_device *dev, char *buf)
> +{
> + struct net_device *netdev = to_net_dev(dev);
> + if (netif_running(netdev)) {
> + return sprintf(buf, fmt_dec, !!netif_carrier_ok(netdev));
> + }
> + return -EINVAL;
> +}
> +
> static CLASS_DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
> static CLASS_DEVICE_ATTR(broadcast, S_IRUGO, show_broadcast, NULL);
> +static CLASS_DEVICE_ATTR(carrier, S_IRUGO, show_carrier, NULL);
>
> /* read-write attributes */
> NETDEVICE_SHOW(mtu, fmt_dec);
> @@ -186,6 +196,7 @@
> &class_device_attr_type,
> &class_device_attr_address,
> &class_device_attr_broadcast,
> + &class_device_attr_carrier,
> NULL
> };
Looks great, it kind of shows how easy sysfs is but how baroque it is as
well.
Hello!
I am just a bit tired and a bit confused, but
Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > The most concise format of this would be:
> > > return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
wouldn't this potentially open a security hole / buffer overflow?
I am currently not really seeing where buf is passed from and what
dec_fmt is, but am I totally wrong?
Nico
--
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org
On Tue, 2004-09-28 at 22:54 +0200, Nico Schottelius wrote:
> Hello!
>
> I am just a bit tired and a bit confused, but
>
> Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > > The most concise format of this would be:
> > > > return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
>
> wouldn't this potentially open a security hole / buffer overflow?
buf comes from fs/sysfs/file.c:fill_read_buf and is PAGESIZE.
Since netif_carrier_ok() returns 0 or 1, don't see how that could ever
turn into a security hole.
> I am currently not really seeing where buf is passed from and what
> dec_fmt is, but am I totally wrong?
dec_fmt is in net-sysfs.c and is defined as "%d\n" to avoid
repetition of same string literal.
Thanks for enlightening [1] me!
Nico
P.S.: [1] enlightenment.org is still alive, just wanted to note that..
Stephen Hemminger [Tue, Sep 28, 2004 at 03:24:43PM -0700]:
> On Tue, 2004-09-28 at 22:54 +0200, Nico Schottelius wrote:
> > Hello!
> >
> > I am just a bit tired and a bit confused, but
> >
> > Jesper Juhl [Tue, Sep 28, 2004 at 09:23:25PM +0200]:
> > > > > Using snprintf in this way is kind of silly. since buffer is PAGESIZE.
> > > > > The most concise format of this would be:
> > > > > return sprintf(buf, dec_fmt, !!netif_carrier_ok(dev));
> >
> > wouldn't this potentially open a security hole / buffer overflow?
>
> buf comes from fs/sysfs/file.c:fill_read_buf and is PAGESIZE.
> Since netif_carrier_ok() returns 0 or 1, don't see how that could ever
> turn into a security hole.
>
>
> > I am currently not really seeing where buf is passed from and what
> > dec_fmt is, but am I totally wrong?
>
> dec_fmt is in net-sysfs.c and is defined as "%d\n" to avoid
> repetition of same string literal.
>
>
--
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org
> On Fri, 22 Oct 2004, Jesper Juhl wrote:
> [sysfs kernel patch]
> Good news,
> Linus has merged the patch in 2.6.10-rc1 so unless someone finds a flaw
> with it or otherwise complains about it it should be in 2.6.10 once that
> is released.
I tested it in 2.6.9 and it works without any problems. I even
included it in an isntallation script and tested pluggin cable
out and in, works like charme.
Nico
--
Keep it simple & stupid, use what's available.
Please use pgp encryption: 8D0E 27A4 is my id.
http://nico.schotteli.us | http://linux.schottelius.org