Re: [PATCH] staging: rtl8188eu: prevent ->ssid overflow in rtw_wx_set_scan()

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Mar 05, 2021 at 10:58:17AM -0600, Edmundo Carmona Antoranz wrote:
> On Fri, Mar 5, 2021 at 2:59 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > -                                       if (sec_len > 0 && sec_len <= len) {
> > +                                       if (sec_len > 0 &&
> > +                                           sec_len <= len &&
> > +                                           sec_len <= 32) {
> 
> I wonder if this could be reduced to (sec_len > 0 && sec_len <=
> min(len, 32)) from a stylistic POV?

I kind of prefer it the way I wrote it.  I prefer conditions split
apart and done ploddingly, one at a time...  You'll notice how I could
have written it like:

					if (sec_len > 0 && sec_len <= len &&
					    sec_len <= 32) {

But I really like my conditions to be spelled out so the "sec_len" is
perfectly aligned in each part of the condition.  Your way would be to
combine two conditions into one part of a line and seems sneaky.

> 
> First attempt at something kernel related so I know there's plenty of
> things to learn (including patterns for problems like this and
> etiquette).

It's good that you're reviewing code...  We try to be predictable though
and no one would have predicted your response.  Ideally patch review
should be like, "Ugh!  Why didn't I think of that?  Of course, we should
propagate the error code."  Or "Oh, I didn't know checkpatch warns about
that."

The truth is that I don't always agree with all of Greg's reviews.  He
is more strict than I am about breaking up patches into multiple things.
(It's a tricky line to define for me).  But I can always predict what
Greg will say in a review so that saves time when I know which patches
he will accept and which he won't.

regards,
dan carpenter