linux-kernel - Re: [PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but not used

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 15 Jun 2010 01:30:48 -0700
From:	"Justin P. Mattock" <justinmattock@...il.com>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
CC:	Valdis.Kletnieks@...edu, linux-kernel@...r.kernel.org,
	Peter Stuge <peter@...ge.se>,
	tpmdd-devel@...ts.sourceforge.net,
	Debora Velarde <debora@...ux.vnet.ibm.com>,
	Rajiv Andrade <srajiv@...ux.vnet.ibm.com>,
	Marcel Selhorst <m.selhorst@...rix.com>
Subject: Re: [PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but	not
 used

On 06/15/2010 12:56 AM, Stefan Richter wrote:
> (adding Cc: tpm maintainers, after I had cut the Cc list earlier...)
>
> Justin P. Mattock wrote:
>> On 06/14/2010 11:57 PM, Stefan Richter wrote:
>>> Justin P. Mattock wrote:
>>>> On 06/14/2010 10:29 PM, Peter Stuge wrote:
>>>>> Justin P. Mattock wrote:
>>>>>> I have no manual in front of me. Did a quick google, but came up with
>>>>>> (no hits) info on what that function does. grep showed too many
>>>>>> entries
>>>>>> to really see why/what this is.
>>>>>
>>>>> Check out the tool cscope. (Or kscope, if you prefer a GUI.)
>>>>
>>>> thanks for this tool.. I think this is what I need.. running around not
>>>> knowing what/where the manual is for a call is a bit daunting.
>>>
>>> Similar to cscope/kscope but online:
>>> http://lxr.free-electrons.com/
>>> http://lxr.linux.no/linux
>>>
>>> E.g. http://lxr.free-electrons.com/ident?i=transmit_cmd
>>
>> hmm.. yeah I use those guys a lot.. with grep also.. I think what might
>> throw me off is things like def.orig = def.new then def.orig gets lost
>> in the mix(or it's there, but the grep becomes gigantic in code
>> results.) main thing I look for is googling "somedefinition linux man"
>> for some idea of what that function does.
>>
>> Justin P. Mattock
>
> I'm afraid that even if there was inline documentation of the function,
> it would still be near impossible to guess what to do with unused return
> codes if one is not familiar with the hardware and with the driver.
>
> To quote your original patch again:
>
>> Im getting this warning when compiling:
>>   CC      drivers/char/tpm/tpm.o
>> drivers/char/tpm/tpm.c: In function 'tpm_gen_interrupt':
>> drivers/char/tpm/tpm.c:508:10: warning: variable 'rc' set but not used
>>
>> The below patch gets rid of the warning,
>> but I'm not sure if it's the best solution.
>>
>>   Signed-off-by: Justin P. Mattock<justinmattock@...il.com>
>>
>> ---
>>   drivers/char/tpm/tpm.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
>> index 05ad4a1..3d685dc 100644
>> --- a/drivers/char/tpm/tpm.c
>> +++ b/drivers/char/tpm/tpm.c
>> @@ -514,6 +514,8 @@ void tpm_gen_interrupt(struct tpm_chip *chip)
>>
>>   	rc = transmit_cmd(chip,&tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
>>   			"attempting to determine the timeouts");
>> +	if (!rc)
>> +		rc = 0;
>>   }
>>   EXPORT_SYMBOL_GPL(tpm_gen_interrupt);
>
> So, void tpm_gen_interrupt() itself clearly is not prepared to do
> anything about an error return from transmit_cmd.  The question is, is
> this OK and can the "rc = " simply be removed, or should
> tpm_gen_interrupt gain some error handling, or should tpm_gen_interrupt
> be changed to a non-void function and its callers check for errors in
> tpm_gen_interrupt?
>

still not sure what that mechanism is doing..

I was going by the warning message i.g.  set but not used
so as soon as it was being used even by adding printk, gcc was happy.
But shutting it off seemed a better approach why? because at the time I 
knew it would work, or fixed it temporarily until somebody who's code 
this is can look at it and send out a better solution(hence what you did).

> It could very well be that there is really no need or no possibility for
> error handling.  But until somebody familiar with the matter has figured
> this out, it is in fact better to leave the compiler warning in place.
>

yep.. I agree..

> Just silencing the compiler if the possibility exists that there is an
> actual flaw here is not the way to go.  Still, good that you reported it
> even with an RFC patch, since the authors were obviously unaware of such
> a warning until now.

I did not like the idea of silencing it.. due to the fact that it can be 
forgotten(or lost in the sands of time per-see)
best I just file a bug report then..in these types of cases.

In any case I still have more warnings..(and that's just my .config)
who knows how many more there are.. If you guys like I can go ahead in 
the next few days and send out another set(minus the rc = 0; thing that 
I did) then go from there.

keep in mind I'm just tackling the ones I think are easy.. ones with 
double variables I left until later down the line..

Justin P. Mattock
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/