| Message ID | 20141003093505.GA7393@mwanda |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > Most people sending checkpatch.pl fixes don't know how to verify the > alignment. This checkpatch warning just encourages newbies to try > introduce bugs. Patch submitters tell us that they just sed the code > and it's the job for the maintainer to check that it's correct. I haven't seen many instances of bad patch submittals on netdev. Is this mostly an issue for staging? Maybe a downgrade to CHK requiring --strict is OK. > If you want to work on these then you can get the same information by > typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN` That's not much of an argument for or against anything in checkpatch as every operation done by it can be reproduced by a series of grep operations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 3 Oct 2014, Joe Perches wrote: > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > > Most people sending checkpatch.pl fixes don't know how to verify the > > alignment. This checkpatch warning just encourages newbies to try > > introduce bugs. Patch submitters tell us that they just sed the code > > and it's the job for the maintainer to check that it's correct. > > I haven't seen many instances of bad patch submittals > on netdev. Is this mostly an issue for staging? > > Maybe a downgrade to CHK requiring --strict is OK. > > > If you want to work on these then you can get the same information by > > typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN` > > That's not much of an argument for or against anything in > checkpatch as every operation done by it can be reproduced > by a series of grep operations. I think it is too bad to have a piece of knowledge that was apparent be made more obscure. Why not just change the checkpatch warning to make more explicit that a lot of expertise is required to make the change? julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 03, 2014 at 07:22:27AM -0700, Joe Perches wrote: > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > > Most people sending checkpatch.pl fixes don't know how to verify the > > alignment. This checkpatch warning just encourages newbies to try > > introduce bugs. Patch submitters tell us that they just sed the code > > and it's the job for the maintainer to check that it's correct. > > I haven't seen many instances of bad patch submittals > on netdev. Is this mostly an issue for staging? I don't follow netdev so I can't say. Most of the time data is aligned at a 4 byte mark so probably you are just getting lucky. I really doubt that netdev checkpatch newbies know about alignment... > > Maybe a downgrade to CHK requiring --strict is OK. I would actually like to turn --strict by default in staging. Checkpatch is a good concept, but it should only do safe things instead of telling newbies to send buggy patches. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-10-03 at 16:30 +0200, Julia Lawall wrote: > On Fri, 3 Oct 2014, Joe Perches wrote: > > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > > > Most people sending checkpatch.pl fixes don't know how to verify the > > > alignment. This checkpatch warning just encourages newbies to try > > > introduce bugs. Patch submitters tell us that they just sed the code > > > and it's the job for the maintainer to check that it's correct. > > > > I haven't seen many instances of bad patch submittals > > on netdev. Is this mostly an issue for staging? > > > > Maybe a downgrade to CHK requiring --strict is OK. [] > I think it is too bad to have a piece of knowledge that was apparent be > made more obscure. Why not just change the checkpatch warning to make > more explicit that a lot of expertise is required to make the change? Any wordsmithing appreciated. Maybe something like: from: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2) to: Where both Ethernet addresses are guaranteed to be __aligned(2), prefer ether_addr_copy() over memcpy() That won't stop people from blindly following any message. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-10-03 at 17:47 +0300, Dan Carpenter wrote: > On Fri, Oct 03, 2014 at 07:22:27AM -0700, Joe Perches wrote: > > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > > > Most people sending checkpatch.pl fixes don't know how to verify the > > > alignment. This checkpatch warning just encourages newbies to try > > > introduce bugs. Patch submitters tell us that they just sed the code > > > and it's the job for the maintainer to check that it's correct. And that's where it's the maintainer's job to educate, inform, reject > > I haven't seen many instances of bad patch submittals > > on netdev. Is this mostly an issue for staging? > > I don't follow netdev so I can't say. > > Most of the time data is aligned at a 4 byte mark so probably you are > just getting lucky. All typical Ethernet frames have 1 of the 2 addresses on an even byte boundary but not 4 byte aligned. Most all of the is_<foo>_ether_addr tests assume __aligned(2) > I really doubt that netdev checkpatch newbies know > about alignment... I think that's a learning opportunity... > > Maybe a downgrade to CHK requiring --strict is OK. > > I would actually like to turn --strict by default in staging. I recall a suggested patch for that. > Checkpatch is a good concept, but it should only do safe things instead > of telling newbies to send buggy patches. checkpatch isn't just for the inexperienced. It's an oversight tester and a style tool. Degrading it doesn't make it better. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 3 Oct 2014, Joe Perches wrote: > On Fri, 2014-10-03 at 16:30 +0200, Julia Lawall wrote: > > On Fri, 3 Oct 2014, Joe Perches wrote: > > > On Fri, 2014-10-03 at 12:35 +0300, Dan Carpenter wrote: > > > > Most people sending checkpatch.pl fixes don't know how to verify the > > > > alignment. This checkpatch warning just encourages newbies to try > > > > introduce bugs. Patch submitters tell us that they just sed the code > > > > and it's the job for the maintainer to check that it's correct. > > > > > > I haven't seen many instances of bad patch submittals > > > on netdev. Is this mostly an issue for staging? > > > > > > Maybe a downgrade to CHK requiring --strict is OK. > [] > > I think it is too bad to have a piece of knowledge that was apparent be > > made more obscure. Why not just change the checkpatch warning to make > > more explicit that a lot of expertise is required to make the change? > > Any wordsmithing appreciated. Maybe something like: > > from: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2) > to: Where both Ethernet addresses are guaranteed to be __aligned(2), prefer ether_addr_copy() over memcpy() > > That won't stop people from blindly following any message. Perhaps add: Note that checking whether the addresses are aligned may require knowing properties of all possible architectures on which the code may be executed. Make this change only if you have this information. julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 52a223e..d540dd4 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4698,16 +4698,6 @@ sub process { } } -# Check for memcpy(foo, bar, ETH_ALEN) that could be ether_addr_copy(foo, bar) - if ($^V && $^V ge 5.10.0 && - $line =~ /^\+(?:.*?)\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/s) { - if (WARN("PREFER_ETHER_ADDR_COPY", - "Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)\n" . $herecurr) && - $fix) { - $fixed[$fixlinenr] =~ s/\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/ether_addr_copy($2, $7)/; - } - } - # typecasts on min/max could be min_t/max_t if ($^V && $^V ge 5.10.0 && defined $stat &&
Most people sending checkpatch.pl fixes don't know how to verify the alignment. This checkpatch warning just encourages newbies to try introduce bugs. Patch submitters tell us that they just sed the code and it's the job for the maintainer to check that it's correct. If you want to work on these then you can get the same information by typing `grep -nw memcpy drivers/net/ -R | grep ETH_ALEN` Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html