From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756046AbbJISpc (ORCPT ); Fri, 9 Oct 2015 14:45:32 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:45323 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753164AbbJISpa (ORCPT ); Fri, 9 Oct 2015 14:45:30 -0400 X-Sasl-enc: M1CFcJYkzSoAXdvzvJT+hFFCMO0xSAaySYfi73mpXCGi 1444416329 Date: Fri, 9 Oct 2015 21:45:27 +0300 From: Sergei Zviagintsev To: David Herrmann Cc: Greg Kroah-Hartman , Daniel Mack , David Herrmann , Djalal Harouni , linux-kernel Subject: Re: [PATCH 25/44] kdbus: Cleanup kdbus_cmd_conn_info() Message-ID: <20151009184527.GJ2189@localhost.localdomain> References: <71c2d1fea4ee6858de74fed74012d78e57596fdd.1444302968.git.sergei@s15v.net> 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:38:11PM +0200, David Herrmann wrote: > Hi > > On Thu, Oct 8, 2015 at 1:31 PM, Sergei Zviagintsev wrote: > > - Move `entry' and `owner' to the scope where they are used. Drop > > redundand initialization of `entry'. Use conditional operator to set > > the value of `owner'. > > > > - Set `ret' to zero right after call to kdbus_pool_slice_copy_kvec(), > > not in the end of function. > > > > - Remove redundant goto. > > > > Signed-off-by: Sergei Zviagintsev > > --- > > ipc/kdbus/connection.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c > > index b3c5f20a57d8..6a73ac3f444d 100644 > > --- a/ipc/kdbus/connection.c > > +++ b/ipc/kdbus/connection.c > > @@ -1702,8 +1702,6 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) > > { > > struct kdbus_meta_conn *conn_meta = NULL; > > struct kdbus_pool_slice *slice = NULL; > > - struct kdbus_name_entry *entry = NULL; > > - struct kdbus_name_owner *owner = NULL; > > struct kdbus_conn *owner_conn = NULL; > > struct kdbus_item *meta_items = NULL; > > struct kdbus_info info = {}; > > @@ -1739,9 +1737,12 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) > > name = argv[1].item ? argv[1].item->str : NULL; > > > > if (name) { > > + struct kdbus_name_entry *entry; > > + struct kdbus_name_owner *owner; > > + > > entry = kdbus_name_lookup_unlocked(bus->name_registry, name); > > - if (entry) > > - owner = kdbus_name_get_owner(entry); > > + owner = entry ? kdbus_name_get_owner(entry) : NULL; > > + > > This looks good to me. > > > if (!owner || > > !kdbus_conn_policy_see_name(conn, current_cred(), name) || > > (cmd->id != 0 && owner->conn->id != cmd->id)) { > > @@ -1804,17 +1805,14 @@ int kdbus_cmd_conn_info(struct kdbus_conn *conn, void __user *argp) > > ret = kdbus_pool_slice_copy_kvec(slice, 0, kvec, cnt, size); > > if (ret < 0) > > goto exit; > > + ret = 0; > > > > kdbus_pool_slice_publish(slice, &cmd->offset, &cmd->info_size); > > > > if (kdbus_member_set_user(&cmd->offset, argp, typeof(*cmd), offset) || > > kdbus_member_set_user(&cmd->info_size, argp, > > - typeof(*cmd), info_size)) { > > + typeof(*cmd), info_size)) > > ret = -EFAULT; > > - goto exit; > > - } > > - > > - ret = 0; > > Again, why? Now you have a random "ret = 0;" somewhere in between, > instead of directly at the tail of the success-path. This change was intended to stress to one who is reading the code that kdbus_pool_slice_copy_kvec() returns >= 0 on success (as it returns the number of bytes copied in contrast to most of functions which return 0 on success). The only reason we have 'ret = 0' in the end of the function is to reset `ret' after kdbus_pool_slice_copy_kvec(), so I decided to group it together. Without kdbus_pool_slice_copy_kvec() we could return `ret' as is, because it is always zero on success path. BTW, kdbus_node_link() for example does the same: uses `ret' to handle the return val of ida_simple_get() and then reset it to zero immediately. But that's too much words on such a simple change. I don't mind omitting it from v2. > > Thanks > David > > > > > exit: > > up_read(&bus->name_registry->rwlock); > > -- > > 1.8.3.1 > >