net/e1000: fix memcpy in e1000_get_strings

PATCH In review Roel Kluin 2 participants 3 messages Nov 29, 2007 – Dec 5, 2007 Network drivers

lore

Share & tools

lore

Roel Kluin

author

2007-11-29 01:54 UTC #0 lore raw

PATCH 1/1 net/e1000: fix memcpy in e1000_get_strings

drivers/net/e1000/e1000_ethtool.c:113:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN

drivers/net/e1000e/ethtool.c:106:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN

E1000_TEST_LEN*ETH_GSTRING_LEN will expand to sizeof(e1000_gstrings_test) / (ETH_GSTRING_LEN * ETH_GSTRING_LEN)

Please confirm that the change is as wanted.
--
A lack of parentheses around defines causes unexpected results due to operator precedences.

Signed-off-by Roel Kluin <[email protected]>

---

drivers/net/e1000/e1000_ethtool.c +1 -1

index 667f18b..b83ccce 100644
@@ -1923,7 +1923,7 @@ e1000_get_strings(struct net_device *netdev, uint32_t stringset, uint8_t *data)
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+			sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {

drivers/net/e1000e/ethtool.c +1 -1

index 6a39784..338c49d 100644
@@ -1739,7 +1739,7 @@ static void e1000_get_strings(struct net_device *netdev, u32 stringset,
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+				sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {

Kok, Auke

2007-11-29 16:29 UTC #1 lore raw

Quoted 10 lines · Roel Kluin

drivers/net/e1000/e1000_ethtool.c:113:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN
drivers/net/e1000e/ethtool.c:106:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN
E1000_TEST_LEN*ETH_GSTRING_LEN will expand to sizeof(e1000_gstrings_test) / (ETH_GSTRING_LEN * ETH_GSTRING_LEN)
Please confirm that the change is as wanted.

Thanks,

I'll take a look at this closely and post these to netdev-2.6.

thanks

Auke

Quoted 37 lines

--
A lack of parentheses around defines causes unexpected results due to operator precedences.
Signed-off-by Roel Kluin <[email protected]>
---
drivers/net/e1000/e1000_ethtool.c +1 -1
index 667f18b..b83ccce 100644
@@ -1923,7 +1923,7 @@ e1000_get_strings(struct net_device *netdev, uint32_t stringset, uint8_t *data)
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+			sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {
drivers/net/e1000e/ethtool.c +1 -2
index 6a39784..338c49d 100644
@@ -1739,7 +1739,7 @@ static void e1000_get_strings(struct net_device *netdev, u32 stringset,
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+				sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Kok, Auke

2007-12-05 19:25 UTC #2 lore raw

In reply to Roel Kluin

Quoted 34 lines · Roel Kluin

drivers/net/e1000/e1000_ethtool.c:113:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN
drivers/net/e1000e/ethtool.c:106:
#define E1000_TEST_LEN sizeof(e1000_gstrings_test) / ETH_GSTRING_LEN
E1000_TEST_LEN*ETH_GSTRING_LEN will expand to sizeof(e1000_gstrings_test) / (ETH_GSTRING_LEN * ETH_GSTRING_LEN)
Please confirm that the change is as wanted.
--
A lack of parentheses around defines causes unexpected results due to operator precedences.
Signed-off-by Roel Kluin <[email protected]>
---
drivers/net/e1000/e1000_ethtool.c +1 -1
index 667f18b..b83ccce 100644
@@ -1923,7 +1923,7 @@ e1000_get_strings(struct net_device *netdev, uint32_t stringset, uint8_t *data)
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+			sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {
drivers/net/e1000e/ethtool.c +0 -0
index 6a39784..338c49d 100644

drivers/net/e1000e/ethtool.c static void e1000_get_strings(struct net_device *netdev, u32 stringset, · line 1745

Quoted 8 lines

drivers/net/e1000e/ethtool.c +1 -1

@@ -1739,7 +1739,7 @@ static void e1000_get_strings(struct net_device *netdev, u32 stringset,
 	switch (stringset) {
 	case ETH_SS_TEST:
 		memcpy(data, *e1000_gstrings_test,
-			E1000_TEST_LEN*ETH_GSTRING_LEN);
+				sizeof(e1000_gstrings_test));
 		break;
 	case ETH_SS_STATS:
 		for (i = 0; i < E1000_GLOBAL_STATS_LEN; i++) {

actually this was recently fixed (with a cleanup to use ARRAY_SIZE!), but I really like the sizeof() safety in the memcpy anyway, so I'll push this patch anyway.

thanks,

Auke