linux-kernel - Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage

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]

Message-Id: <1245254936.5982.261.camel@desktop>
Date:	Wed, 17 Jun 2009 09:08:56 -0700
From:	Daniel Walker <dwalker@...o99.com>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
Cc:	Greg Kroah-Hartman <greg@...ah.com>,
	Brian Swetland <swetland@...gle.com>, arve@...roid.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] staging: android: binder: Remove some funny
	&&	usage

On Wed, 2009-06-17 at 08:28 -0700, Jeremy Fitzhardinge wrote:
> On 06/17/09 07:37, Daniel Walker wrote:
> > I agree it's reasonable in some cases.. The reason I changed this is
> > because at first glance I didn't know what those lines were suppose to
> > do. The equals signs all bleed together combined with the length of the
> > statement makes it not match other similar usage. The if statement just
> > makes the whole thing explicit.
> >    
> 
> I definitely see your point, but the if() statement variant has the 
> downside of only conditionally assigning the variable, and requiring it 
> to be initialized separately.  I have a general code-cleanup rule to 
> convert:
> 
> 	foo = false;
> 	if (something_is_true())
> 		foo = true;
> 
> to
> 
> 	foo = something_is_true();

Above seems more like a speed up, rather than a clean up. I would think
it's likely fine for a lot of cases tho.

> 
> Maybe a bit of reformatting and some tactical use of parens would help?
> 
> 	wait_for_proc_work = (!thread->transaction_stack&&  list_empty(&thread->todo));
> 
> (I'm not normally a fan of NULL-as-false, but it reads OK here.)

I'm ok with this change for the first of the two, but the second one is
too long.. However, I reviewed the code a little more and I think the
wait_for_proc_work variable could likely get eliminated in the second
case. There is something racy going on wrt. to the variable in the
second case. So it probably better for me to just drop the second change
in hopes of a more detailed cleanup.

> > Not to mention this code is a mess, very dense, and has little or no
> > comments. Anything that can be done to make the code more clear, seem
> > like a cleanup to me.
> >    
> 
> No argument from me.  Not to mention that I have no idea from reading 
> the code what the whole subsystem is for; "Android IPC Subsystem" 
> doesn't tell me much, other than a gnawing feeling about having yet 
> another IPC subsystem to deal with.

I was hoping Brian could explain this. I also added Arve (the author) to
the CC list. Maybe they can explain the purpose of the subsystem.

> > As for using "bool" , AFAIK that's only part of C++ ..
> >    
> 
> No, it is also C99, and becoming widely used in the kernel.

Was this a recent change to C99, cause my compiler still doesn't know
about it .. I also see a couple places in the kernel where bool is
getting typedef'ed or somehow declared..

Daniel

--
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/