Messages in this thread |  | | Date | Thu, 05 Dec 2013 18:35:13 +0000 | From | Serban Constantinescu <> | Subject | Re: [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
|  |