[PATCH 0/3] kdbus: minor readability improvements

[PATCH 0/3] kdbus: minor readability improvements

[Sergei Zviagintsev]

Little improvements to make things easier to read.

Sergei Zviagintsev (3):
  kdbus: kdbus_reply_find(): return on found entry
  kdbus: optimize error path in kdbus_reply_new()
  kdbus: optimize if statements in kdbus_conn_disconnect()

 ipc/kdbus/connection.c | 15 +++++++--------
 ipc/kdbus/reply.c      | 22 +++++++++-------------
 2 files changed, 16 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] kdbus: kdbus_reply_find(): return on found entry

[Sergei Zviagintsev]

Return found entry immediately instead of assigning it to additional
variable and breaking the loop. It's simpler to read, the same way is
used in kdbus_conn_has_name(), for example.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/reply.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/ipc/kdbus/reply.c b/ipc/kdbus/reply.c
index 89d355b44f63..9d823ebee71f 100644
--- a/ipc/kdbus/reply.c
+++ b/ipc/kdbus/reply.c
@@ -171,17 +171,15 @@ struct kdbus_reply *kdbus_reply_find(struct kdbus_conn *replying,
 				     struct kdbus_conn *reply_dst,
 				     u64 cookie)
 {
-	struct kdbus_reply *r, *reply = NULL;
+	struct kdbus_reply *r;
 
 	list_for_each_entry(r, &reply_dst->reply_list, entry) {
 		if (r->cookie == cookie &&
-		    (!replying || r->reply_src == replying)) {
-			reply = r;
-			break;
-		}
+		    (!replying || r->reply_src == replying))
+			return r;
 	}
 
-	return reply;
+	return NULL;
 }
 
 /**
-- 
1.8.3.1

Re: [PATCH 1/3] kdbus: kdbus_reply_find(): return on found entry

[David Herrmann]

Hi

On Wed, Jun 17, 2015 at 7:14 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Return found entry immediately instead of assigning it to additional
> variable and breaking the loop. It's simpler to read, the same way is
> used in kdbus_conn_has_name(), for example.
>
> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/reply.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Yeah, left-over from reply->mutex.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> diff --git a/ipc/kdbus/reply.c b/ipc/kdbus/reply.c
> index 89d355b44f63..9d823ebee71f 100644
> --- a/ipc/kdbus/reply.c
> +++ b/ipc/kdbus/reply.c
> @@ -171,17 +171,15 @@ struct kdbus_reply *kdbus_reply_find(struct kdbus_conn *replying,
>                                      struct kdbus_conn *reply_dst,
>                                      u64 cookie)
>  {
> -       struct kdbus_reply *r, *reply = NULL;
> +       struct kdbus_reply *r;
>
>         list_for_each_entry(r, &reply_dst->reply_list, entry) {
>                 if (r->cookie == cookie &&
> -                   (!replying || r->reply_src == replying)) {
> -                       reply = r;
> -                       break;
> -               }
> +                   (!replying || r->reply_src == replying))
> +                       return r;
>         }
>
> -       return reply;
> +       return NULL;
>  }
>
>  /**
> --
> 1.8.3.1
>

[PATCH 2/3] kdbus: optimize error path in kdbus_reply_new()

[Sergei Zviagintsev]

Move cleanup code to separate location as it never executes on normal
flow. This removes extra if-block and the need to initialize `ret'.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/reply.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/ipc/kdbus/reply.c b/ipc/kdbus/reply.c
index 9d823ebee71f..e6791d86ec92 100644
--- a/ipc/kdbus/reply.c
+++ b/ipc/kdbus/reply.c
@@ -37,7 +37,7 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
 				    bool sync)
 {
 	struct kdbus_reply *r;
-	int ret = 0;
+	int ret;
 
 	if (atomic_inc_return(&reply_dst->request_count) >
 	    KDBUS_CONN_MAX_REQUESTS_PENDING) {
@@ -64,13 +64,11 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
 		r->waiting = true;
 	}
 
-exit_dec_request_count:
-	if (ret < 0) {
-		atomic_dec(&reply_dst->request_count);
-		return ERR_PTR(ret);
-	}
-
 	return r;
+
+exit_dec_request_count:
+	atomic_dec(&reply_dst->request_count);
+	return ERR_PTR(ret);
 }
 
 static void __kdbus_reply_free(struct kref *kref)
-- 
1.8.3.1

Re: [PATCH 2/3] kdbus: optimize error path in kdbus_reply_new()

[David Herrmann]

Hi

On Wed, Jun 17, 2015 at 7:14 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> Move cleanup code to separate location as it never executes on normal
> flow. This removes extra if-block and the need to initialize `ret'.
>
> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/reply.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> diff --git a/ipc/kdbus/reply.c b/ipc/kdbus/reply.c
> index 9d823ebee71f..e6791d86ec92 100644
> --- a/ipc/kdbus/reply.c
> +++ b/ipc/kdbus/reply.c
> @@ -37,7 +37,7 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
>                                     bool sync)
>  {
>         struct kdbus_reply *r;
> -       int ret = 0;
> +       int ret;
>
>         if (atomic_inc_return(&reply_dst->request_count) >
>             KDBUS_CONN_MAX_REQUESTS_PENDING) {
> @@ -64,13 +64,11 @@ struct kdbus_reply *kdbus_reply_new(struct kdbus_conn *reply_src,
>                 r->waiting = true;
>         }
>
> -exit_dec_request_count:
> -       if (ret < 0) {
> -               atomic_dec(&reply_dst->request_count);
> -               return ERR_PTR(ret);
> -       }
> -
>         return r;
> +
> +exit_dec_request_count:
> +       atomic_dec(&reply_dst->request_count);
> +       return ERR_PTR(ret);
>  }
>
>  static void __kdbus_reply_free(struct kref *kref)
> --
> 1.8.3.1
>

[PATCH 3/3] kdbus: optimize if statements in kdbus_conn_disconnect()

[Sergei Zviagintsev]

if (r->sync) branch and code after it both call kdbus_reply_unlink().
Rewrite them as if-else to eliminate code duplication and make algorithm
more obvious.

Convert outer if statement to use continue in order to reduce
indentation and make things easier to read.

Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
---
 ipc/kdbus/connection.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
index 707be050b408..9993753d11de 100644
--- a/ipc/kdbus/connection.c
+++ b/ipc/kdbus/connection.c
@@ -559,17 +559,16 @@ int kdbus_conn_disconnect(struct kdbus_conn *conn, bool ensure_queue_empty)
 	hash_for_each(bus->conn_hash, i, c, hentry) {
 		mutex_lock(&c->lock);
 		list_for_each_entry_safe(r, r_tmp, &c->reply_list, entry) {
-			if (r->reply_src == conn) {
-				if (r->sync) {
-					kdbus_sync_reply_wakeup(r, -EPIPE);
-					kdbus_reply_unlink(r);
-					continue;
-				}
+			if (r->reply_src != conn)
+				continue;
 
+			if (r->sync)
+				kdbus_sync_reply_wakeup(r, -EPIPE);
+			else
 				/* send a 'connection dead' notification */
 				kdbus_notify_reply_dead(bus, c->id, r->cookie);
-				kdbus_reply_unlink(r);
-			}
+
+			kdbus_reply_unlink(r);
 		}
 		mutex_unlock(&c->lock);
 	}
-- 
1.8.3.1

Re: [PATCH 3/3] kdbus: optimize if statements in kdbus_conn_disconnect()

[David Herrmann]

Hi

On Wed, Jun 17, 2015 at 7:14 PM, Sergei Zviagintsev <sergei@s15v.net> wrote:
> if (r->sync) branch and code after it both call kdbus_reply_unlink().
> Rewrite them as if-else to eliminate code duplication and make algorithm
> more obvious.
>
> Convert outer if statement to use continue in order to reduce
> indentation and make things easier to read.
>
> Signed-off-by: Sergei Zviagintsev <sergei@s15v.net>
> ---
>  ipc/kdbus/connection.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> index 707be050b408..9993753d11de 100644
> --- a/ipc/kdbus/connection.c
> +++ b/ipc/kdbus/connection.c
> @@ -559,17 +559,16 @@ int kdbus_conn_disconnect(struct kdbus_conn *conn, bool ensure_queue_empty)
>         hash_for_each(bus->conn_hash, i, c, hentry) {
>                 mutex_lock(&c->lock);
>                 list_for_each_entry_safe(r, r_tmp, &c->reply_list, entry) {
> -                       if (r->reply_src == conn) {
> -                               if (r->sync) {
> -                                       kdbus_sync_reply_wakeup(r, -EPIPE);
> -                                       kdbus_reply_unlink(r);
> -                                       continue;
> -                               }
> +                       if (r->reply_src != conn)
> +                               continue;
>
> +                       if (r->sync)
> +                               kdbus_sync_reply_wakeup(r, -EPIPE);
> +                       else
>                                 /* send a 'connection dead' notification */
>                                 kdbus_notify_reply_dead(bus, c->id, r->cookie);
> -                               kdbus_reply_unlink(r);
> -                       }
> +
> +                       kdbus_reply_unlink(r);
>                 }
>                 mutex_unlock(&c->lock);
>         }
> --
> 1.8.3.1
>

Re: [PATCH 0/3] kdbus: minor readability improvements

[Djalal Harouni]

Hi,

On Wed, Jun 17, 2015 at 08:14:55PM +0300, Sergei Zviagintsev wrote:
> Little improvements to make things easier to read.
> 
> Sergei Zviagintsev (3):
>   kdbus: kdbus_reply_find(): return on found entry
>   kdbus: optimize error path in kdbus_reply_new()
>   kdbus: optimize if statements in kdbus_conn_disconnect()
Thanks for the patches! all good.


>  ipc/kdbus/connection.c | 15 +++++++--------
>  ipc/kdbus/reply.c      | 22 +++++++++-------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> -- 
> 1.8.3.1
> 

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 0/3] kdbus: minor readability improvements

[Djalal Harouni]

On Wed, Jun 17, 2015 at 08:14:55PM +0300, Sergei Zviagintsev wrote:
> Little improvements to make things easier to read.
> 
> Sergei Zviagintsev (3):
>   kdbus: kdbus_reply_find(): return on found entry
>   kdbus: optimize error path in kdbus_reply_new()
>   kdbus: optimize if statements in kdbus_conn_disconnect()
All three patches:

Reviewed-by: Djalal Harouni <tixxdz@opendz.org>

Thanks!

>  ipc/kdbus/connection.c | 15 +++++++--------
>  ipc/kdbus/reply.c      | 22 +++++++++-------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> -- 
> 1.8.3.1
> 

-- 
Djalal Harouni
http://opendz.org

Re: [PATCH 0/3] kdbus: minor readability improvements

[Greg Kroah-Hartman]

On Wed, Jun 17, 2015 at 08:14:55PM +0300, Sergei Zviagintsev wrote:
> Little improvements to make things easier to read.
> 
> Sergei Zviagintsev (3):
>   kdbus: kdbus_reply_find(): return on found entry
>   kdbus: optimize error path in kdbus_reply_new()
>   kdbus: optimize if statements in kdbus_conn_disconnect()
> 
>  ipc/kdbus/connection.c | 15 +++++++--------
>  ipc/kdbus/reply.c      | 22 +++++++++-------------
>  2 files changed, 16 insertions(+), 21 deletions(-)

All applied, thanks.

greg k-h