rfc: treewide replace local ethernet broadcast char arrays with a global ?

Treewide there are ~60 declarations of a ethernet broadcast
address as a 6 byte array that are later used as either an
output for vsprintf extension %pM or as a source array to
copy or compare.

Perhaps it'd be useful to declare a global static const u8[]
in net somewhere instead to save the text/data space of these
duplicate declarations.

$ grep-2.5.4 -n --include=*.[ch] "\b(?:static\s+)?(?:const\s+)?(?:char|unsigned\s+char|u8)\s+\w+\s*\[\s*(?:ETH_ALEN|6)\s*\]\s*=\s*\{\s*(?:(?i:0xff|255),\s*){5,5}\s*(?i:0xff|255)\s*\}" * | \
  grep -P "\.[ch]:\d+:"

On 03/20/2018 01:00 PM, Joe Perches wrote:

Treewide there are ~60 declarations of a ethernet broadcast
address as a 6 byte array that are later used as either an
output for vsprintf extension %pM or as a source array to
copy or compare.

Perhaps it'd be useful to declare a global static const u8[]
in net somewhere instead to save the text/data space of these
duplicate declarations.

I could have sworn that such a thing existed already within
include/linux/etherdevice.h but it is only eth_reserved_addr_base and
friends as well as is_broadcast_ether_addr(). How about you do it?

$ grep-2.5.4 -n --include=*.[ch] "\b(?:static\s+)?(?:const\s+)?(?:char|unsigned\s+char|u8)\s+\w+\s*\[\s*(?:ETH_ALEN|6)\s*\]\s*=\s*\{\s*(?:(?i:0xff|255),\s*){5,5}\s*(?i:0xff|255)\s*\}" * | \
  grep -P "\.[ch]:\d+:"

-- 
Florian

On Tue, 2018-03-20 at 13:07 -0700, Florian Fainelli wrote:

On 03/20/2018 01:00 PM, Joe Perches wrote:

quoted

Treewide there are ~60 declarations of a ethernet broadcast
address as a 6 byte array that are later used as either an
output for vsprintf extension %pM or as a source array to
copy or compare.

Perhaps it'd be useful to declare a global static const u8[]
in net somewhere instead to save the text/data space of these
duplicate declarations.

I could have sworn that such a thing existed already within
include/linux/etherdevice.h but it is only eth_reserved_addr_base and
friends as well as is_broadcast_ether_addr(). How about you do it?

I hadn't noticed eth_reserved_addr_base before and it does
seem incorrectly specified as static definitions in .h
files for what should be extern are odd.

Real question is, if the global ethernet broadcast address
array is deemed useful, where to put the definition.  The
extern declaration should definitely be in etherdevice.h

Maybe net/ethernet/eth.c ?

There are multiple instances of static const arrays for broadcast
and zero ethernet addresses used for various purposes.

Add const u8 ether_<foo>_addr[ETH_ALEN] globals to consolidate these uses.

Miscellanea:

o Move and rename the eth_reserved_addr_base declaration to this file
  and declare it extern to avoid possible multiple static definitions
o Add compilation to the Makefile

Signed-off-by: Joe Perches <joe@perches.com>
---

Not sure this is the best place for this.  Better ideas welcomed.

 include/linux/etherdevice.h | 12 ++++++++----
 net/core/Makefile           |  3 ++-
 net/core/ether_addrs.c      | 16 ++++++++++++++++
 3 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100644 net/core/ether_addrs.c

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 79563840c295..85d2486b2959 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h

@@ -63,10 +63,14 @@ struct sk_buff **eth_gro_receive(struct sk_buff **head,
 				 struct sk_buff *skb);
 int eth_gro_complete(struct sk_buff *skb, int nhoff);
 
+/* Generic Ethernet addresses */
+extern const u8 ether_broadcast_addr[ETH_ALEN];	/* all 0xff */
+extern const u8 ether_zero_addr[ETH_ALEN];	/* all zeros */
+
 /* Reserved Ethernet Addresses per IEEE 802.1Q */
-static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) =
-{ 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 };
-#define eth_stp_addr eth_reserved_addr_base
+extern const u8 ether_reserved_addr_base[ETH_ALEN];
+
+#define eth_stp_addr ether_reserved_addr_base
 
 /**
  * is_link_local_ether_addr - Determine if given Ethernet address is link-local

@@ -80,7 +84,7 @@ static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) =
 static inline bool is_link_local_ether_addr(const u8 *addr)
 {
 	__be16 *a = (__be16 *)addr;
-	static const __be16 *b = (const __be16 *)eth_reserved_addr_base;
+	static const __be16 *b = (const __be16 *)ether_reserved_addr_base;
 	static const __be16 m = cpu_to_be16(0xfff0);
 
 #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)

diff --git a/net/core/Makefile b/net/core/Makefile
index 6dbbba8c57ae..7fb941243cc3 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile

@@ -4,7 +4,8 @@
 #
 
 obj-y := sock.o request_sock.o skbuff.o datagram.o stream.o scm.o \
-	 gen_stats.o gen_estimator.o net_namespace.o secure_seq.o flow_dissector.o
+	 gen_stats.o gen_estimator.o net_namespace.o secure_seq.o \
+	 flow_dissector.o ether_addrs.o
 
 obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
 

diff --git a/net/core/ether_addrs.c b/net/core/ether_addrs.c
new file mode 100644
index 000000000000..18eedabe73fa
--- /dev/null
+++ b/net/core/ether_addrs.c

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Various const global Ethernet addresses */
+
+#include <linux/etherdevice.h>
+
+const u8 ether_broadcast_addr[ETH_ALEN] __aligned(2) = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff
+};
+
+const u8 ether_zero_addr[ETH_ALEN] __aligned(2) = {
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+
+const u8 ether_reserved_addr_base[ETH_ALEN] __aligned(2) = {
+	0x01, 0x80, 0xc2, 0x00, 0x00, 0x00
+};

-- 
2.15.0

From: Joe Perches <joe@perches.com>
Date: Wed, 21 Mar 2018 02:03:37 -0700

There are multiple instances of static const arrays for broadcast
and zero ethernet addresses used for various purposes.

Add const u8 ether_<foo>_addr[ETH_ALEN] globals to consolidate these uses.

Miscellanea:

o Move and rename the eth_reserved_addr_base declaration to this file
  and declare it extern to avoid possible multiple static definitions
o Add compilation to the Makefile

Signed-off-by: Joe Perches <joe@perches.com>
---

Not sure this is the best place for this.  Better ideas welcomed.

Well, for one thing you will need to export the symbols.

As for location, anything referencing these symbols or calling helpers
which do should depend upon ethernet.