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

[Dan Carpenter]

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.

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