From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755654AbbE2K0W (ORCPT ); Fri, 29 May 2015 06:26:22 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:18747 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754098AbbE2K0R (ORCPT ); Fri, 29 May 2015 06:26:17 -0400 Date: Fri, 29 May 2015 13:25:59 +0300 From: Dan Carpenter To: Riley Andrews Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , devel@driverdev.osuosl.org Subject: Re: [PATCH 05/13] android: binder: refactor binder_transact transaction buffer loop Message-ID: <20150529102559.GH28762@mwanda> References: <1432854511-33320-1-git-send-email-riandrews@android.com> <1432854511-33320-6-git-send-email-riandrews@android.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1432854511-33320-6-git-send-email-riandrews@android.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 28, 2015 at 04:08:23PM -0700, Riley Andrews wrote: > +static int binder_transaction_buffer_acquire( > + struct binder_transaction *t, struct binder_transaction_data *tr, > + struct binder_thread *thread, struct binder_transaction *in_reply_to) > +{ > + struct binder_proc *proc = thread->proc; > + binder_size_t *offp, *off_end, off_min; > + struct flat_binder_object *fp; > + uint32_t result; > + > + if (!IS_ALIGNED(tr->offsets_size, sizeof(binder_size_t))) { > + binder_user_error("%d:%d got transaction with invalid offsets size, %lld\n", > + proc->pid, thread->pid, > + (u64)tr->offsets_size); > + return BR_FAILED_REPLY; This smells like a behavior change. In the original code we called trace_binder_transaction_failed_buffer_release(). Are you sure it's correct? Naming the labels after the goto location is an anti-pattern. aaa = kmalloc(); if (!aaa) goto kmalloc_failed; The label name doesn't provide useful information compared to the line before. If binder_transaction_buffer_acquire() fails we say "goto err_translate_failed;" but actually translate didn't fail because we haven't yet attempted to translate so the little information the label does provide is misleading. Grumble grumble etc. Also "error:" is a meaningless label name. Name labels after what the label does "goto release_buffer;". regards, dan carpenter