From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756642AbbJIRw6 (ORCPT ); Fri, 9 Oct 2015 13:52:58 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:42384 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752290AbbJIRw4 (ORCPT ); Fri, 9 Oct 2015 13:52:56 -0400 X-Sasl-enc: h3ubaKrW0/pRiUW6bk2shZ5hHCmwVRMs8jqI9h+C3Nz5 1444413175 Date: Fri, 9 Oct 2015 20:52:54 +0300 From: Sergei Zviagintsev To: David Herrmann Cc: Greg Kroah-Hartman , Daniel Mack , Djalal Harouni , linux-kernel Subject: Re: [PATCH 18/44] kdbus: Add var initialization to kdbus_conn_entry_insert() Message-ID: <20151009175254.GE2189@localhost.localdomain> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Thu, Oct 08, 2015 at 04:28:29PM +0200, David Herrmann wrote: > Hi > > On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev wrote: > > Assign zero to `ret' in the beginning of function instead of doing it > > in the end. > > > > Signed-off-by: Sergei Zviagintsev > > --- > > ipc/kdbus/connection.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c > > index 4f3cd370ecd9..185ed3ba1bce 100644 > > --- a/ipc/kdbus/connection.c > > +++ b/ipc/kdbus/connection.c > > @@ -889,7 +889,7 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src, > > const struct kdbus_name_entry *name) > > { > > struct kdbus_queue_entry *entry; > > - int ret; > > + int ret = 0; > > > > kdbus_conn_lock2(conn_src, conn_dst); > > > > @@ -916,8 +916,6 @@ int kdbus_conn_entry_insert(struct kdbus_conn *conn_src, > > kdbus_queue_entry_enqueue(entry, reply); > > wake_up_interruptible(&conn_dst->wait); > > > > - ret = 0; > > - > > Not a big fan of this. It makes it less obvious, and this style is > wrong in several cases (but not here). We often only check for "ret < > 0", but generally want >0 to be turned into 0 on return. > > It does not matter in this specific case, but I prefer making return > codes explicit, rather than relying on a previous initialization to be > still valid. > > What's your rationale here? The rationale is to keep things simple. That `ret' var is used only once to deliver the error code, and the function itself has only two local vars and fits into my 12.5 inch thinkpad screen, so IMO that extra line with assignment is redundant. I agree that in some cases we need to handle 'ret > 0', but using same templates for every particular case produces boring code :) And BTW we have this style in number of places over kdbus code. For example see kdbus_handle_ioctl_control(), kdbus_handle_ioctl_ep(), kdbus_name_update(), kdbus_name_release(), kdbus_pool_release_offset(), kdbus_pool_slice_copy(). > > Thanks > David > > > exit_unlock: > > kdbus_conn_unlock2(conn_src, conn_dst); > > return ret; > > -- > > 1.8.3.1 > >