[PATCH] Staging: bcm: Fix information leak in ioctl, IOCTL_BCM_REGISTER_READ_PRIVATE

[Dan Carpenter]

On Fri, Oct 28, 2011 at 08:00:49AM -0400, Kevin McKinney wrote:
> In each of the above solutions, shouldn't we set count =
> STATUS_SUCCESS when count is greater then 0?
> 
> count = rdm();
> if (count < 0) {
>         retval = count;
>         .................................
>         .................................
> } else {
>         retval = STATUS_SUCCESS;
>         .........................................
>         .........................................
> }

I wrote the email on the assumption that probably retval is already
STATUS_SUCCESS before we do the call to rdm().  That's normally how
people code.

	good.
	good.
	if (we hit an error)
		return -ERRORCODE;
	good.
	good.

But obviously, this not all code is written like that.  Do what you
have to do.  :)  So long as it doesn't introduce bugs, I'm not going
to be able to complain.

The main point that I was making is that it gets very confusing when
you have retval and status that are almost but not quite the same.
This is a common anti-pattern and I've been guilty of it myself from
time to time.  But using retval and count is easier to keep track of
to say which is which.

Also this driver is obviously a complete mess.  We have Status,
status, retval, ret, and uiRetVal but they should all be just "int
ret;".

Also there are way too many levels of abstraction.  We have
InterfaceRDM() which returns the data and rdmalt() which returns the
data in cpu endian format.  The other three functions should be
removed: rdm(), ->interface_rdm(), BcmRDM().  Not that you have to do
this, but I'm just saying that if someone wanted to do this they
could.  Also the function names suck.

regards,
dan carpenter