lkml.org 
[lkml]   [2013]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/9] staging: android: binder: Move some of the logic into subfunction
On 05/12/13 08:18, 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.

This patch moves code around so that some of it can be shared with the
compat layer. I agree that due to the abundance of debug code it is, at
times, hard to have a clear idea of what is actually happening and adds
extra obscurities to the logic.

Thanks,
Serban C



\
 
 \ /
  Last update: 2013-12-05 20:01    [W:0.178 / U:2.852 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site