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 <[email protected]> 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 <[email protected]> > > --- > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
