Re: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Dec 05, 2013 at 11:18:22AM +0300, Dan Carpenter wrote:
> On Wed, Dec 04, 2013 at 06:09:33PM +0000, Serban Constantinescu wrote:
> > +static void bc_increfs_done(struct binder_proc *proc,
> > +		struct binder_thread *thread, uint32_t cmd,
> > +		void __user *node_ptr, void __user *cookie)
> > +{
> > +	struct binder_node *node;
> > +
> > +	node = binder_get_node(proc, node_ptr);
> > +	if (node == NULL) {
> > +		binder_user_error("%d:%d %s u%p no match\n",
> > +			proc->pid, thread->pid,
> > +			cmd == BC_INCREFS_DONE ?
> > +			"BC_INCREFS_DONE" :
> > +			"BC_ACQUIRE_DONE",
> > +			node_ptr);
> > +		return;
> > +	}
> > +	if (cookie != node->cookie) {
> > +		binder_user_error("%d:%d %s u%p node %d cookie mismatch %p != %p\n",
> > +			proc->pid, thread->pid,
> > +			cmd == BC_INCREFS_DONE ?
> > +			"BC_INCREFS_DONE" : "BC_ACQUIRE_DONE",
> > +			node_ptr, node->debug_id,
> > +			cookie, node->cookie);
> > +		return;
> > +	}
> > +	if (cmd == BC_ACQUIRE_DONE) {
> > +		if (node->pending_strong_ref == 0) {
> > +			binder_user_error("%d:%d BC_ACQUIRE_DONE node %d has no pending acquire request\n",
> > +				proc->pid, thread->pid,
> > +				node->debug_id);
> > +			return;
> > +		}
> > +		node->pending_strong_ref = 0;
> > +	} else {
> > +		if (node->pending_weak_ref == 0) {
> > +			binder_user_error("%d:%d BC_INCREFS_DONE node %d has no pending increfs request\n",
> > +				proc->pid, thread->pid,
> > +				node->debug_id);
> > +			return;
> > +		}
> > +		node->pending_weak_ref = 0;
> > +	}
> > +	binder_dec_node(node, cmd == BC_ACQUIRE_DONE, 0);
> > +	binder_debug(BINDER_DEBUG_USER_REFS,
> > +		     "%d:%d %s node %d ls %d lw %d\n",
> > +		     proc->pid, thread->pid,
> > +		     cmd == BC_INCREFS_DONE ?
> > +		     "BC_INCREFS_DONE" :
> > +		     "BC_ACQUIRE_DONE",
> > +		     node->debug_id, node->local_strong_refs,
> > +		     node->local_weak_refs);
> > +	return;
> > +}
> 
> This function is 52 lines long but at least 32 of those lines are
> debug code.
> 
> Just extra of lines of code means you have increased the number of bugs
> 150%.  But what I know is that from a static analysis perspective, debug
> code has more defects per line then regular code it's worse than that.
> Plus all the extra noise obscures the actual logic and makes the real
> code annoying to look at so it's worse still.
> 
> If you want to move this stuff out of staging, then remove all the extra
> crap so it doesn't look like barf.

I don't ever think the binder code will be moved out of staging for a
variety of reasons, but that doesn't mean it shouldn't be cleaned up :)

thanks,

greg k-h