Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Johannes Berg]

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