Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Thu, 2015-04-23 at 16:54 -0700, Joe Perches wrote: > On Thu, 2015-04-23 at 21:53 +0200, Mateusz Kulikowski wrote: > > I noticed funny behavior about $stat matches - > > it reports the same error several times (including as "scope" whole file) > > Is it feature or "feature" or I missed something? > > You have to make sure the first character of $stat is a + > > if ($stat =~ /\+(?:.*)\bmem Make that if ($stat =~ /^\+(?:.*)\bmem -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Thu, 2015-04-23 at 21:53 +0200, Mateusz Kulikowski wrote:
> On 22.04.2015 00:27, Joe Perches wrote:
> > On Tue, 2015-04-21 at 23:44 +0200, Mateusz Kulikowski wrote:
> >> On 21.04.2015 23:22, Joe Perches wrote:
> >>> On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote:
> >> (...)
> (...)
> >> True, True; If you prefer $line and ability to --fix - I'll use that in v3
> >
> > I suppose you could do both $line and $stat
> > and the fix would only work when it's on a
> > single line.
> >
> > Perhaps something like this would work:
> >
> > if ($line =~ /whatever/ ||
> > (defined($stat) && $stat =~ /whatever/)) {
> > if (WARN(...) &&
> > $fix) {
> > fixed[$fixlinenr] =~ s/whatever/appropriate/;
> > }
> > }
>
> Isn't it enough to just match $stat and do fix for line (that in
> some cases will just not match)?
Yeah, that'd work too.
> One more thing
> I noticed funny behavior about $stat matches -
> it reports the same error several times (including as "scope" whole file)
> Is it feature or "feature" or I missed something?
You have to make sure the first character of $stat is a +
if ($stat =~ /\+(?:.*)\bmem
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On 22.04.2015 00:27, Joe Perches wrote:
> On Tue, 2015-04-21 at 23:44 +0200, Mateusz Kulikowski wrote:
>> On 21.04.2015 23:22, Joe Perches wrote:
>>> On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote:
>> (...)
(...)
>> True, True; If you prefer $line and ability to --fix - I'll use that in v3
>
> I suppose you could do both $line and $stat
> and the fix would only work when it's on a
> single line.
>
> Perhaps something like this would work:
>
> if ($line =~ /whatever/ ||
> (defined($stat) && $stat =~ /whatever/)) {
> if (WARN(...) &&
> $fix) {
> fixed[$fixlinenr] =~ s/whatever/appropriate/;
> }
> }
Isn't it enough to just match $stat and do fix for line (that in
some cases will just not match)?
One more thing
I noticed funny behavior about $stat matches -
it reports the same error several times (including as "scope" whole file)
Is it feature or "feature" or I missed something?
Ex. file:
-- cut
int foo(void)
{
baz();
memset(a, b, 0);
bar();
}
-- cut
Output of (@master)
-- cut
$ scripts/checkpatch.pl -f test42.c --types MEMSET
ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd
#1: FILE: test42.c:1:
+int foo(void)
{
baz();
memset(a, b, 0);
bar();
}
ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd
#2: FILE: test42.c:2:
+{
baz();
memset(a, b, 0);
bar();
}
ERROR: memset to 0's uses 0 as the 2nd argument, not the 3rd
#4: FILE: test42.c:4:
+memset(a, b, 0);
total: 3 errors, 0 warnings, 6 lines checked
NOTE: Used message types: MEMSET
test42.c has style problems, please review.
If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
--cut
Regards,
Mateusz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Tue, 2015-04-21 at 23:44 +0200, Mateusz Kulikowski wrote:
> On 21.04.2015 23:22, Joe Perches wrote:
> > On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote:
> (...)
> >>
> >> Perhaps it would be smarter to use (for both patches) $stat instead.
> >> This applies also to existing checks (like PREFER_ETHER_ADDR_COPY)
> >> so we can catch calls formatted like
> >>
> >> memset(very.long.structure->something.something_different42,
> >>0xFF, ETH_ALEN);
> >
> > Yes, likely that's true.
> >
> > checkpatch couldn't --fix it easily unless it's on a
> > single line though.
>
> True, True; If you prefer $line and ability to --fix - I'll use that in v3
I suppose you could do both $line and $stat
and the fix would only work when it's on a
single line.
Perhaps something like this would work:
if ($line =~ /whatever/ ||
(defined($stat) && $stat =~ /whatever/)) {
if (WARN(...) &&
$fix) {
fixed[$fixlinenr] =~ s/whatever/appropriate/;
}
}
No worries about getting 'round the the list.
It'll get got eventually.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On 21.04.2015 23:22, Joe Perches wrote: > On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote: (...) >> >> Perhaps it would be smarter to use (for both patches) $stat instead. >> This applies also to existing checks (like PREFER_ETHER_ADDR_COPY) >> so we can catch calls formatted like >> >> memset(very.long.structure->something.something_different42, >>0xFF, ETH_ALEN); > > Yes, likely that's true. > > checkpatch couldn't --fix it easily unless it's on a > single line though. True, True; If you prefer $line and ability to --fix - I'll use that in v3 > > As far as I can tell, there are ~120 of these "memcpy"s > in the tree, but there aren't any "memset"s like that > split into 2 or more lines. Some of them are probably candidates for ether_addr_copy(_unaligned) :) I'll probably take a look at them once I have both functions available. Regards, Mateusz > > Here's a list of the multiple line memcpy(..., ETH_ALEN) > uses that I found. > > arch/arm/mach-davinci/board-mityomapl138.c:144: > memcpy(soc_info->emac_pdata->mac_addr, > factory_config.mac, ETH_ALEN); > drivers/staging/rtl8712/rtl8712_recv.c:395: > memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->src, > ETH_ALEN); > drivers/staging/rtl8712/rtl8712_recv.c:396: > memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->dst, > ETH_ALEN); > drivers/staging/rtl8712/rtl8712_recv.c:402: > memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->src, > ETH_ALEN); > drivers/staging/rtl8712/rtl8712_recv.c:403: > memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->dst, > ETH_ALEN); > drivers/staging/rtl8712/os_intfs.c:398: > memcpy(pnetdev->dev_addr, > padapter->eeprompriv.mac_addr, ETH_ALEN); > drivers/staging/rtl8712/os_intfs.c:414: > memcpy(padapter->eeprompriv.mac_addr, > pnetdev->dev_addr, ETH_ALEN); > drivers/staging/rtl8712/rtl871x_ioctl_linux.c:107: > memcpy(wrqu.ap_addr.sa_data, pmlmepriv->cur_network.network.MacAddress, > ETH_ALEN); > drivers/staging/rtl8712/rtl871x_ioctl_linux.c:838: > memcpy(psecuritypriv->PMKIDList[psecuritypriv-> > PMKIDIndex].Bssid, strIssueBssid, ETH_ALEN); > drivers/staging/rtl8712/rtl871x_xmit.c:489: > memcpy(pwlanhdr->addr1, get_bssid(pmlmepriv), > ETH_ALEN); > drivers/staging/rtl8712/rtl871x_xmit.c:496: > memcpy(pwlanhdr->addr2, get_bssid(pmlmepriv), > ETH_ALEN); > drivers/staging/rtl8712/rtl871x_xmit.c:503: > memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv), > ETH_ALEN); > drivers/staging/rtl8712/rtl871x_xmit.c:507: > memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv), > ETH_ALEN); > drivers/staging/rtl8192e/rtllib_softmac_wx.c:125: > memcpy(wrqu->ap_addr.sa_data, > ieee->current_network.bssid, ETH_ALEN); > drivers/staging/rtl8192e/rtllib_tx.c:697: > memcpy(&header.addr1, ieee->current_network.bssid, > ETH_ALEN); > drivers/staging/rtl8192e/rtllib_tx.c:700: > memcpy(&header.addr3, > ieee->current_network.bssid, ETH_ALEN); > drivers/staging/rtl8192e/rtllib_tx.c:709: > memcpy(&header.addr3, ieee->current_network.bssid, > ETH_ALEN); > drivers/staging/rtl8192e/rtllib_softmac.c:3748: > memcpy(wrqu.ap_addr.sa_data, ieee->current_network.bssid, > ETH_ALEN); > drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c:126: > memcpy(wrqu->ap_addr.sa_data, > ieee->current_network.bssid, ETH_ALEN); > drivers/staging/slicoss/slicoss.c:567: > memcpy(adapter->currmacaddr, adapter->macaddr, > ETH_ALEN); > drivers/staging/slicoss/slicoss.c:569: > memcpy(adapter->netdev->dev_addr, adapter->currmacaddr, > ETH_ALEN); > drivers/staging/rtl8723au/hal/usb_halinit.c:1020: > memcpy(pEEPROM->mac_addr, &hwinfo[EEPROM_MAC_ADDR_8723AU], > ETH_ALEN); > drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:327: > memcpy(pwlanhdr->addr1, > get_my_bssid23a(&pmlmeinfo->network), ETH_ALEN); > drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:328: > memcpy(pwlanhdr->addr2, myid(&padapter->eeprompriv), > ETH_ALEN); > drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:335: > memcpy(pwlanhdr->addr2, > get_my
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Tue, 2015-04-21 at 22:57 +0200, Mateusz Kulikowski wrote:
> Hi Joe,
>
> On 20.04.2015 03:13, Joe Perches wrote:
> > On Mon, 2015-04-20 at 00:16 +0200, Mateusz Kulikowski wrote:
> >> Suggest using eth_zero_addr() or eth_broadcast_addr() instead of memset().
> >
> > Hi again Mateusz
> >
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> >> @@ -5042,6 +5042,22 @@ sub process {
> >> "Prefer ether_addr_equal() or
> >> ether_addr_equal_unaligned() over memcmp()\n" . $herecurr)
> >>}
> >>
> >> +# check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr
> >> +# check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr
> >> + if ($^V && $^V ge 5.10.0 &&
> >> + $line =~
> >> /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s)
> >> {
> >
> > Because you are working with $line and not $stat,
> > the last /s isn't useful here.
> >
> > $line is always a single line.
>
> Perhaps it would be smarter to use (for both patches) $stat instead.
> This applies also to existing checks (like PREFER_ETHER_ADDR_COPY)
> so we can catch calls formatted like
>
> memset(very.long.structure->something.something_different42,
>0xFF, ETH_ALEN);
Yes, likely that's true.
checkpatch couldn't --fix it easily unless it's on a
single line though.
As far as I can tell, there are ~120 of these "memcpy"s
in the tree, but there aren't any "memset"s like that
split into 2 or more lines.
Here's a list of the multiple line memcpy(..., ETH_ALEN)
uses that I found.
arch/arm/mach-davinci/board-mityomapl138.c:144:
memcpy(soc_info->emac_pdata->mac_addr,
factory_config.mac, ETH_ALEN);
drivers/staging/rtl8712/rtl8712_recv.c:395:
memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->src,
ETH_ALEN);
drivers/staging/rtl8712/rtl8712_recv.c:396:
memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->dst,
ETH_ALEN);
drivers/staging/rtl8712/rtl8712_recv.c:402:
memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->src,
ETH_ALEN);
drivers/staging/rtl8712/rtl8712_recv.c:403:
memcpy(skb_push(sub_skb, ETH_ALEN), pattrib->dst,
ETH_ALEN);
drivers/staging/rtl8712/os_intfs.c:398:
memcpy(pnetdev->dev_addr,
padapter->eeprompriv.mac_addr, ETH_ALEN);
drivers/staging/rtl8712/os_intfs.c:414:
memcpy(padapter->eeprompriv.mac_addr,
pnetdev->dev_addr, ETH_ALEN);
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:107:
memcpy(wrqu.ap_addr.sa_data, pmlmepriv->cur_network.network.MacAddress,
ETH_ALEN);
drivers/staging/rtl8712/rtl871x_ioctl_linux.c:838:
memcpy(psecuritypriv->PMKIDList[psecuritypriv->
PMKIDIndex].Bssid, strIssueBssid, ETH_ALEN);
drivers/staging/rtl8712/rtl871x_xmit.c:489:
memcpy(pwlanhdr->addr1, get_bssid(pmlmepriv),
ETH_ALEN);
drivers/staging/rtl8712/rtl871x_xmit.c:496:
memcpy(pwlanhdr->addr2, get_bssid(pmlmepriv),
ETH_ALEN);
drivers/staging/rtl8712/rtl871x_xmit.c:503:
memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv),
ETH_ALEN);
drivers/staging/rtl8712/rtl871x_xmit.c:507:
memcpy(pwlanhdr->addr3, get_bssid(pmlmepriv),
ETH_ALEN);
drivers/staging/rtl8192e/rtllib_softmac_wx.c:125:
memcpy(wrqu->ap_addr.sa_data,
ieee->current_network.bssid, ETH_ALEN);
drivers/staging/rtl8192e/rtllib_tx.c:697:
memcpy(&header.addr1, ieee->current_network.bssid,
ETH_ALEN);
drivers/staging/rtl8192e/rtllib_tx.c:700:
memcpy(&header.addr3,
ieee->current_network.bssid, ETH_ALEN);
drivers/staging/rtl8192e/rtllib_tx.c:709:
memcpy(&header.addr3, ieee->current_network.bssid,
ETH_ALEN);
drivers/staging/rtl8192e/rtllib_softmac.c:3748:
memcpy(wrqu.ap_addr.sa_data, ieee->current_network.bssid,
ETH_ALEN);
drivers/staging/rtl8192u/ieee80211/ieee80211_softmac_wx.c:126:
memcpy(wrqu->ap_addr.sa_data,
ieee->current_network.bssid, ETH_ALEN);
drivers/staging/slicoss/slicoss.c:567:
memcpy(adapter->currmacaddr, adapter->macaddr,
ETH_ALEN);
drivers/staging/slicoss/slicoss.c:569:
memcpy(adapter->netdev->dev_addr, adapter->currmacaddr,
ETH_ALEN);
drivers/staging/rtl8723au/hal/usb_halinit.c:1020:
memcpy(p
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
Hi Joe,
On 20.04.2015 03:13, Joe Perches wrote:
> On Mon, 2015-04-20 at 00:16 +0200, Mateusz Kulikowski wrote:
>> Suggest using eth_zero_addr() or eth_broadcast_addr() instead of memset().
>
> Hi again Mateusz
>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -5042,6 +5042,22 @@ sub process {
>> "Prefer ether_addr_equal() or
>> ether_addr_equal_unaligned() over memcmp()\n" . $herecurr)
>> }
>>
>> +# check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr
>> +# check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr
>> +if ($^V && $^V ge 5.10.0 &&
>> +$line =~
>> /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s) {
>
> Because you are working with $line and not $stat,
> the last /s isn't useful here.
>
> $line is always a single line.
Perhaps it would be smarter to use (for both patches) $stat instead.
This applies also to existing checks (like PREFER_ETHER_ADDR_COPY)
so we can catch calls formatted like
memset(very.long.structure->something.something_different42,
0xFF, ETH_ALEN);
Regards,
Mateusz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
On Mon, 2015-04-20 at 00:16 +0200, Mateusz Kulikowski wrote:
> Suggest using eth_zero_addr() or eth_broadcast_addr() instead of memset().
Hi again Mateusz
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5042,6 +5042,22 @@ sub process {
>"Prefer ether_addr_equal() or
> ether_addr_equal_unaligned() over memcmp()\n" . $herecurr)
> }
>
> +# check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr
> +# check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr
> + if ($^V && $^V ge 5.10.0 &&
> + $line =~
> /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s) {
Because you are working with $line and not $stat,
the last /s isn't useful here.
$line is always a single line.
> + my $ms_val = $7;
> +
> + if ($ms_val =~ /^(0x|)0+$/i) {
It's trivially faster to use (?:0x|) so the 0x is not captured.
> + WARN("PREFER_ETH_ZERO_ADDR",
> + "Prefer eth_zero_addr over memset()\n" .
> $herecurr);
And these could be:
if (WARN(...) &&
$fix) {
$fixed[$fixlinenr] =
s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_zero_addr($1)/;
}
}
> + } elsif ($ms_val =~ /^(?:0xff|255)$/i) {
> + WARN("PREFER_ETH_BROADCAST_ADDR",
> + "Prefer eth_broadcast_addr() over
> memset()\n" . $herecurr);
if (WARN(...) &&
$fix) {
$fixed[$fixlinenr] =
s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_broadcast_addr($1)/;
}
cheers, Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/2] checkpatch: suggest using eth_zero_addr() and eth_broadcast_addr()
Suggest using eth_zero_addr() or eth_broadcast_addr() instead of memset().
Signed-off-by: Mateusz Kulikowski
---
scripts/checkpatch.pl | 16
1 file changed, 16 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b9ea436..aee6d43 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5042,6 +5042,22 @@ sub process {
"Prefer ether_addr_equal() or
ether_addr_equal_unaligned() over memcmp()\n" . $herecurr)
}
+# check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr
+# check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr
+ if ($^V && $^V ge 5.10.0 &&
+ $line =~
/^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s) {
+
+ my $ms_val = $7;
+
+ if ($ms_val =~ /^(0x|)0+$/i) {
+ WARN("PREFER_ETH_ZERO_ADDR",
+"Prefer eth_zero_addr over memset()\n" .
$herecurr);
+ } elsif ($ms_val =~ /^(?:0xff|255)$/i) {
+ WARN("PREFER_ETH_BROADCAST_ADDR",
+"Prefer eth_broadcast_addr() over
memset()\n" . $herecurr);
+ }
+ }
+
# typecasts on min/max could be min_t/max_t
if ($^V && $^V ge 5.10.0 &&
defined $stat &&
--
1.8.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
