Re: [PATCH v2] staging: rtl8712: remove unused variable from rtl871x_mlme.c

From: Sergei Krainov <sergei.krainov.lkd@gmail.com>
To: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
	florian.c.schilhabel@googlemail.com,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH v2] staging: rtl8712: remove unused variable from rtl871x_mlme.c
Date: Thu, 8 Apr 2021 17:29:50 +0200	[thread overview]
Message-ID: <20210408152950.GB5306@test-VirtualBox> (raw)
In-Reply-To: <CAOc6eta6ehHRhEYrWv0daS6WrC4oJg0Q8q2CB=K5BzF-E61jxQ@mail.gmail.com>

On Thu, Apr 08, 2021 at 08:29:00AM -0600, Edmundo Carmona Antoranz wrote:
> On Thu, Apr 8, 2021 at 6:10 AM Sergei Krainov
> <sergei.krainov.lkd@gmail.com> wrote:
> > No side effects can be seen locally or in r8712_find_network()
> 
> I am sorry to jump in. Sergei, what Greg is asking is basically why
> you want to delete the r8712_find_network call in the first place.
> Deleting an unused variable is fine, but the problem here is that you
> are _also_ deleting a call to a function that _probably_ does some
> things that, even if you want to get rid of the variable, you would
> probably like to keep on doing (and so the call would remain). Is that
> call really not doing anything relevant? That's what you will have to
> explain in the patch in order for it to make sense.
> 
> As a side note on top of the question about the call, it's not like
> the variable is not being used. It's used right after the call to
> r8712_find_network to check the result of the call... so is the real
> purpose of the patch to remove the call in the first place and then
> having the variable removed because there is no point in having it if
> the call goes away?
> 
> I hope that helps.
Thank you for clarification, guess I really misunderstood the question
and didn't explain properly why I'm doing it.

In this block of code call to r8712_find_network() exist only for one
purpose, to return value to the pcur_wlan. And after that we're not
using pcur_wlan.

So in my opinion it looks like a very subtle bug where we have unused
variable, which is allocated by r8712_find_network(), and if that
succeeds we're also modifying value by pcur_wlan->fixed = false;.
And after all that we're not using variable and compiler has no chance
of catching that because of what we're doing with that value.

Please correct me if I'm wrong in something, I just found that
questionable behavior and decided to send patch, so someone can see
it and say if I'm wrong or not. In case I'm right this change can be
_possibly_ accepted.

Also sorry for asking here, but is it okay that my commit has [PATCH v2]
and subject is [PATCH v2] in mutt, but in mailing list I still see
[PATCH]?

Greg, thanks a lot for reviews of my code you did in past few days, I
really appreciate that, but just didn't want to flood mailing list with
my appreciation only.