[PATCH 0/5] UM: Fine-tuning for some function implementations

[PATCH 0/5] UM: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 22:48:02 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  port: Move an assignment for the variable "fd" in port_wait()
  port: Delete three error messages for a failed memory allocation
  port: Improve size determinations in port_data()
  ubd: Move two assignments for the variable "err" in ubd_remove()
  ubd: Improve size determinations in do_ubd_request()

 arch/um/drivers/port_kern.c | 23 ++++++++---------------
 arch/um/drivers/ubd_kern.c  | 18 +++++++++---------
 2 files changed, 17 insertions(+), 24 deletions(-)

-- 
2.11.0

[PATCH 1/5] um: port: Move an assignment for the variable "fd" in port_wait()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 21:40:29 +0100

A local variable was set to an error code in one case before a concrete
error situation was detected. Thus move the corresponding assignment into
an if branch to indicate a software failure there.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/um/drivers/port_kern.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/um/drivers/port_kern.c b/arch/um/drivers/port_kern.c
index 40ca5cc275e9..b2bbda21c5f3 100644
--- a/arch/um/drivers/port_kern.c
+++ b/arch/um/drivers/port_kern.c
@@ -230,10 +230,10 @@ int port_wait(void *data)
 
 	atomic_inc(&port->wait_count);
 	while (1) {
-		fd = -ERESTARTSYS;
-		if (wait_for_completion_interruptible(&port->done))
+		if (wait_for_completion_interruptible(&port->done)) {
+			fd = -ERESTARTSYS;
 			goto out;
-
+		}
 		spin_lock(&port->lock);
 
 		conn = list_entry(port->connections.next, struct connection,
-- 
2.11.0

Re: [PATCH 1/5] um: port: Move an assignment for the variable "fd" in port_wait()

[Jeff Dike]

On Wed, Jan 18, 2017 at 10:56:17PM +0100, SF Markus Elfring wrote:
> diff --git a/arch/um/drivers/port_kern.c b/arch/um/drivers/port_kern.c
> index 40ca5cc275e9..b2bbda21c5f3 100644
> --- a/arch/um/drivers/port_kern.c
> +++ b/arch/um/drivers/port_kern.c
> @@ -230,10 +230,10 @@ int port_wait(void *data)
>  
>  	atomic_inc(&port->wait_count);
>  	while (1) {
> -		fd = -ERESTARTSYS;
> -		if (wait_for_completion_interruptible(&port->done))
> +		if (wait_for_completion_interruptible(&port->done)) {
> +			fd = -ERESTARTSYS;
>  			goto out;
> -
> +		}
>  		spin_lock(&port->lock);
>  
>  		conn = list_entry(port->connections.next, struct connection,
> -- 
> 2.11.0
>

The current code is pretty standard kernel coding style.  The inline
    fd = -ERESTARTSYS;
is likely free, or close to it, and I believe that having anything
besides a goto inside the if could make it significantly more
expensive.

				Jeff

-- 
Jeff Dike
AddToIt
978-254-0789 (o)
978-394-8986 (c)

[PATCH 2/5] um: port: Delete three error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 22:00:14 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: Possible unnecessary 'out of memory' message

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/um/drivers/port_kern.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/arch/um/drivers/port_kern.c b/arch/um/drivers/port_kern.c
index b2bbda21c5f3..c96741e920f5 100644
--- a/arch/um/drivers/port_kern.c
+++ b/arch/um/drivers/port_kern.c
@@ -87,11 +87,8 @@ static int port_accept(struct port_list *port)
 	}
 
 	conn = kmalloc(sizeof(*conn), GFP_ATOMIC);
-	if (conn == NULL) {
-		printk(KERN_ERR "port_accept : failed to allocate "
-		       "connection\n");
+	if (!conn)
 		goto out_close;
-	}
 	*conn = ((struct connection)
 		{ .list 	= LIST_HEAD_INIT(conn->list),
 		  .fd 		= fd,
@@ -170,10 +167,8 @@ void *port_data(int port_num)
 			goto found;
 	}
 	port = kmalloc(sizeof(struct port_list), GFP_KERNEL);
-	if (port == NULL) {
-		printk(KERN_ERR "Allocation of port list failed\n");
+	if (!port)
 		goto out;
-	}
 
 	fd = port_listen_fd(port_num);
 	if (fd < 0) {
@@ -202,10 +197,8 @@ void *port_data(int port_num)
 
  found:
 	dev = kmalloc(sizeof(struct port_dev), GFP_KERNEL);
-	if (dev == NULL) {
-		printk(KERN_ERR "Allocation of port device entry failed\n");
+	if (!dev)
 		goto out;
-	}
 
 	*dev = ((struct port_dev) { .port  		= port,
 				    .helper_pid  	= -1,
-- 
2.11.0

Re: [PATCH 2/5] um: port: Delete three error messages for a failed memory allocation

[Jeff Dike]

> --- a/arch/um/drivers/port_kern.c
> +++ b/arch/um/drivers/port_kern.c
> @@ -87,11 +87,8 @@ static int port_accept(struct port_list *port)
>  	}
>  
>  	conn = kmalloc(sizeof(*conn), GFP_ATOMIC);
> -	if (conn == NULL) {
> -		printk(KERN_ERR "port_accept : failed to allocate "
> -		       "connection\n");
> +	if (!conn)
>  		goto out_close;
> -	}
>  	*conn = ((struct connection)
>  		{ .list 	= LIST_HEAD_INIT(conn->list),
>  		  .fd 		= fd,

I don't see how this eliminates a possible error.  It should behave
exactly the same.  To me, this is an expressiveness issue.

!x is something you use with something that is conceptually a Boolean.

x == NULL is a question about a pointer, which is the case here.

				Jeff
-- 
Jeff Dike
AddToIt
978-254-0789 (o)
978-394-8986 (c)

Re: [PATCH 2/5] um: port: Delete three error messages for a failed memory allocation

[SF Markus Elfring]

>> +++ b/arch/um/drivers/port_kern.c
>> @@ -87,11 +87,8 @@ static int port_accept(struct port_list *port)
>>  	}
>>  
>>  	conn = kmalloc(sizeof(*conn), GFP_ATOMIC);
>> -	if (conn == NULL) {
>> -		printk(KERN_ERR "port_accept : failed to allocate "
>> -		       "connection\n");
>> +	if (!conn)
>>  		goto out_close;
>> -	}
>>  	*conn = ((struct connection)
>>  		{ .list 	= LIST_HEAD_INIT(conn->list),
>>  		  .fd 		= fd,
> 
> I don't see how this eliminates a possible error.

The suggested change affects three coding style issues at this place.

* Repetition of an out-of-memory message

  See also:
  http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf

* Unwanted splitting of a message string

* Usage of a specific preprocessor symbol


> !x is something you use with something that is conceptually a Boolean.

Pointers can be also treated in this way, can't they?

Regards,
Markus

[PATCH 3/5] um: port: Improve size determinations in port_data()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 22:20:14 +0100

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/um/drivers/port_kern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/port_kern.c b/arch/um/drivers/port_kern.c
index c96741e920f5..60c1b43a8381 100644
--- a/arch/um/drivers/port_kern.c
+++ b/arch/um/drivers/port_kern.c
@@ -166,7 +166,7 @@ void *port_data(int port_num)
 		if (port->port == port_num)
 			goto found;
 	}
-	port = kmalloc(sizeof(struct port_list), GFP_KERNEL);
+	port = kmalloc(sizeof(*port), GFP_KERNEL);
 	if (!port)
 		goto out;
 
@@ -196,7 +196,7 @@ void *port_data(int port_num)
 	list_add(&port->list, &ports);
 
  found:
-	dev = kmalloc(sizeof(struct port_dev), GFP_KERNEL);
+	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		goto out;
 
-- 
2.11.0

[PATCH 4/5] um: ubd: Move two assignments for the variable "err" in ubd_remove()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 22:23:18 +0100

A local variable was set to an error code in two cases before a concrete
error situation was detected. Thus move the corresponding assignments into
if branches to indicate a software failure there.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/um/drivers/ubd_kern.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 85410279beab..6d686f735538 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -1049,19 +1049,21 @@ static int ubd_remove(int n, char **error_out)
 {
 	struct gendisk *disk = ubd_gendisk[n];
 	struct ubd *ubd_dev;
-	int err = -ENODEV;
+	int err;
 
 	mutex_lock(&ubd_lock);
 
 	ubd_dev = &ubd_devs[n];
-
-	if(ubd_dev->file == NULL)
+	if (!ubd_dev->file) {
+		err = -ENODEV;
 		goto out;
+	}
 
 	/* you cannot remove a open disk */
-	err = -EBUSY;
-	if(ubd_dev->count > 0)
+	if (ubd_dev->count > 0) {
+		err = -EBUSY;
 		goto out;
+	}
 
 	ubd_gendisk[n] = NULL;
 	if(disk != NULL){
-- 
2.11.0

[PATCH 5/5] um: ubd: Improve size determinations in do_ubd_request()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 18 Jan 2017 22:38:04 +0100

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/um/drivers/ubd_kern.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 6d686f735538..50327d5a9a01 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -1386,8 +1386,7 @@ static void do_ubd_request(struct request_queue *q)
 		req = dev->request;
 
 		if (req_op(req) == REQ_OP_FLUSH) {
-			io_req = kmalloc(sizeof(struct io_thread_req),
-					 GFP_ATOMIC);
+			io_req = kmalloc(sizeof(*io_req), GFP_ATOMIC);
 			if (io_req == NULL) {
 				if (list_empty(&dev->restart))
 					list_add(&dev->restart, &restart);
@@ -1401,8 +1400,7 @@ static void do_ubd_request(struct request_queue *q)
 		while(dev->start_sg < dev->end_sg){
 			struct scatterlist *sg = &dev->sg[dev->start_sg];
 
-			io_req = kmalloc(sizeof(struct io_thread_req),
-					 GFP_ATOMIC);
+			io_req = kmalloc(sizeof(*io_req), GFP_ATOMIC);
 			if(io_req == NULL){
 				if(list_empty(&dev->restart))
 					list_add(&dev->restart, &restart);
-- 
2.11.0

Re: [PATCH 0/5] UM: Fine-tuning for some function implementations

[Richard Weinberger]

Markus,

Am 18.01.2017 um 22:55 schrieb SF Markus Elfring:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 18 Jan 2017 22:48:02 +0100
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (5):
>   port: Move an assignment for the variable "fd" in port_wait()
>   port: Delete three error messages for a failed memory allocation
>   port: Improve size determinations in port_data()
>   ubd: Move two assignments for the variable "err" in ubd_remove()
>   ubd: Improve size determinations in do_ubd_request()

please don't send drive-by patches.
Please see:
marc.info/?i=20170111225640.14e5d962@bbrezillon

Nacked-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: UM: Fine-tuning for some function implementations

[SF Markus Elfring]

> please don't send drive-by patches.

Would you dare to take another look at the published update steps
in any other software combination?

Regards,
Markus

Re: UM: Fine-tuning for some function implementations

[Anton Ivanov]

How about tackling some real problems and performance issues instead?

There are a few of those in the network, interrupt and memory 
subsystems. Take your pick.

A.


On 19/01/17 17:13, SF Markus Elfring wrote:
>> please don't send drive-by patches.
> Would you dare to take another look at the published update steps
> in any other software combination?
>
> Regards,
> Markus
>

Re: [PATCH 0/5] UM: Fine-tuning for some function implementations

[Jeff Dike]

The kmalloc(sizeof(struct foo), ...) => kmalloc(sizeof(*foo), ...)
ones are OK.

The rest is cargo-cult programming.

				Jeff
-- 
Jeff Dike
AddToIt
978-254-0789 (o)
978-394-8986 (c)