lkml.org 
[lkml]   [2018]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] wireless: mark expected switch fall-throughs
From
Date
On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote:
> On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> >
> > On 10/23/18 9:01 AM, Johannes Berg wrote:
> > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
> > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > > where we are expecting to fall through.
> > > >
> > > > Warning level 3 was used: -Wimplicit-fallthrough=3
> > > >
> > > > This code was not tested and GCC 7.2.0 was used to compile it.
> > >
> > > Look, I'm not going to make this any clearer: I'm not applying patches
> > > like that where you've invested no effort whatsoever on verifying that
> > > they're correct.
> > >
> >
> > How do you suggest me to verify that every part is correct in this type
> > of patches?
> >
>
> BTW... I'm under the impression you think that I don't even look at
> the code. Is that correct?

That's what your commit log looks like, yes. This is your full commit
log:

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Warning level 3 was used: -Wimplicit-fallthrough=3

This code was not tested and GCC 7.2.0 was used to compile it.

For all I know, you could've run spatch to add the comments wherever
there was no break, and then compiled it once.

> I've been working on this for quite a while, and in every case I try to
> understand the code in terms of the context in which every warning is
> reported.

That's great.

> Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>
> 5690 case WLAN_CIPHER_SUITE_CCMP:
> 5691 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> 5692 break;
> 5693 case WLAN_CIPHER_SUITE_TKIP:
> 5694 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
> 5695 default:
> 5696 return -EOPNOTSUPP;
> 5697 }

Indeed, that looks like a bug, although kinda benign since it just means
TKIP will always use software crypto and TKIP is slow anyway ;-)

> I do this analysis for every warning. Now, when I say I haven't tested the code
> is because I don't have any log as evidence for anything. Not that I haven't put
> any effort on trying to understand it and its context. When I started working on
> this task there were more than 2000 of these issues, now there are around 600 left.
>
> I have fixed many bugs on the way, so a good amount of work is being invested on
> this, and it's paying off. :)

:-)

> Now, let me ask you this question:
>
> It would be easier for you to review this patch if I turn it into a series?
>
> I can do that without a problem.

I'd be happy if you were to actually just mention that you've looked at
them, and found them to be expected fall throughs. I'll still review
them, but without that information I feel like I'm doing the first round
of reviews this code ever got.

johannes

\
 
 \ /
  Last update: 2018-10-23 22:35    [W:0.046 / U:17.244 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site