[PATCH 0/4] block-virtio: Fine-tuning for two function implementations

[PATCH 0/4] block-virtio: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 14:05:05 +0200

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

Markus Elfring (4):
  Use kmalloc_array() in init_vq()
  Less function calls in init_vq() after error detection
  Delete an unnecessary initialisation in init_vq()
  Rename a jump label in virtblk_get_id()

 drivers/block/virtio_blk.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

-- 
2.10.0

[PATCH 1/4] virtio_blk: Use kmalloc_array() in init_vq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 11:32:22 +0200

Multiplications for the size determination of memory allocations
indicated that array data structures should be processed.
Thus use the corresponding function "kmalloc_array".

This issue was detected by using the Coccinelle software.

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

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 93b1aaa..6553eb7 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -390,13 +390,13 @@ static int init_vq(struct virtio_blk *vblk)
 	if (err)
 		num_vqs = 1;
 
-	vblk->vqs = kmalloc(sizeof(*vblk->vqs) * num_vqs, GFP_KERNEL);
+	vblk->vqs = kmalloc_array(num_vqs, sizeof(*vblk->vqs), GFP_KERNEL);
 	if (!vblk->vqs)
 		return -ENOMEM;
 
-	names = kmalloc(sizeof(*names) * num_vqs, GFP_KERNEL);
-	callbacks = kmalloc(sizeof(*callbacks) * num_vqs, GFP_KERNEL);
-	vqs = kmalloc(sizeof(*vqs) * num_vqs, GFP_KERNEL);
+	names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL);
+	callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL);
+	vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL);
 	if (!names || !callbacks || !vqs) {
 		err = -ENOMEM;
 		goto out;
-- 
2.10.0

[PATCH 2/4] virtio_blk: Less function calls in init_vq() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 13:20:44 +0200

The kfree() function was called in up to three cases
by the init_vq() function during error handling even if
the passed variable contained a null pointer.

* Split a condition check for memory allocation failures.

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 6553eb7..d28dbcf 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -395,11 +395,21 @@ static int init_vq(struct virtio_blk *vblk)
 		return -ENOMEM;
 
 	names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL);
+	if (!names) {
+		err = -ENOMEM;
+		goto free_vblk_vqs;
+	}
+
 	callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL);
+	if (!callbacks) {
+		err = -ENOMEM;
+		goto free_names;
+	}
+
 	vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL);
-	if (!names || !callbacks || !vqs) {
+	if (!vqs) {
 		err = -ENOMEM;
-		goto out;
+		goto free_callbacks;
 	}
 
 	for (i = 0; i < num_vqs; i++) {
@@ -411,19 +421,21 @@ static int init_vq(struct virtio_blk *vblk)
 	/* Discover virtqueues and write information to configuration.  */
 	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
 	if (err)
-		goto out;
+		goto free_vqs;
 
 	for (i = 0; i < num_vqs; i++) {
 		spin_lock_init(&vblk->vqs[i].lock);
 		vblk->vqs[i].vq = vqs[i];
 	}
 	vblk->num_vqs = num_vqs;
-
-out:
+ free_vqs:
 	kfree(vqs);
+ free_callbacks:
 	kfree(callbacks);
+ free_names:
 	kfree(names);
 	if (err)
+ free_vblk_vqs:
 		kfree(vblk->vqs);
 	return err;
 }
-- 
2.10.0

[PATCH 3/4] virtio_blk: Delete an unnecessary initialisation in init_vq()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 13:43:50 +0200

The local variable "err" will be set to an appropriate value
by a following statement.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/block/virtio_blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index d28dbcf..696f452 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -376,7 +376,7 @@ static void virtblk_config_changed(struct virtio_device *vdev)
 
 static int init_vq(struct virtio_blk *vblk)
 {
-	int err = 0;
+	int err;
 	int i;
 	vq_callback_t **callbacks;
 	const char **names;
-- 
2.10.0

[PATCH 4/4] virtio_blk: Rename a jump label in virtblk_get_id()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 13 Sep 2016 13:50:56 +0200

Adjust a jump label according to the current Linux coding style convention.

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

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 696f452..fef2bd0 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -247,10 +247,10 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
 
 	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
 	if (err)
-		goto out;
+		goto put_request;
 
 	err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
-out:
+ put_request:
 	blk_put_request(req);
 	return err;
 }
-- 
2.10.0

Re: [PATCH 2/4] virtio_blk: Less function calls in init_vq() after error detection

[Christian Borntraeger]

On 09/13/2016 02:13 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 13 Sep 2016 13:20:44 +0200
> 
> The kfree() function was called in up to three cases
> by the init_vq() function during error handling even if
> the passed variable contained a null pointer.
> 
> * Split a condition check for memory allocation failures.
> 
> * Adjust jump targets according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)

Can't you see from this diffstat that the patch actually seems to makes
the code more complex?
In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
    virtio_blk: Fix a slient kernel panic

which did the opposite of your patch. And in fact it fixed a bug. Quite obviously
multiple labels are harder to read and harder to get right. For error handling with
just kfree one label is just the right thing to.

> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 6553eb7..d28dbcf 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -395,11 +395,21 @@ static int init_vq(struct virtio_blk *vblk)
>  		return -ENOMEM;
> 
>  	names = kmalloc_array(num_vqs, sizeof(*names), GFP_KERNEL);
> +	if (!names) {
> +		err = -ENOMEM;
> +		goto free_vblk_vqs;
> +	}
> +
>  	callbacks = kmalloc_array(num_vqs, sizeof(*callbacks), GFP_KERNEL);
> +	if (!callbacks) {
> +		err = -ENOMEM;
> +		goto free_names;
> +	}
> +
>  	vqs = kmalloc_array(num_vqs, sizeof(*vqs), GFP_KERNEL);
> -	if (!names || !callbacks || !vqs) {
> +	if (!vqs) {
>  		err = -ENOMEM;
> -		goto out;
> +		goto free_callbacks;
>  	}
> 
>  	for (i = 0; i < num_vqs; i++) {
> @@ -411,19 +421,21 @@ static int init_vq(struct virtio_blk *vblk)
>  	/* Discover virtqueues and write information to configuration.  */
>  	err = vdev->config->find_vqs(vdev, num_vqs, vqs, callbacks, names);
>  	if (err)
> -		goto out;
> +		goto free_vqs;
> 
>  	for (i = 0; i < num_vqs; i++) {
>  		spin_lock_init(&vblk->vqs[i].lock);
>  		vblk->vqs[i].vq = vqs[i];
>  	}
>  	vblk->num_vqs = num_vqs;
> -
> -out:
> + free_vqs:
>  	kfree(vqs);
> + free_callbacks:
>  	kfree(callbacks);
> + free_names:
>  	kfree(names);
>  	if (err)
> + free_vblk_vqs:
>  		kfree(vblk->vqs);
>  	return err;
>  }
> 

Re: virtio_blk: Less function calls in init_vq() after error detection

[SF Markus Elfring]

>>  drivers/block/virtio_blk.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> Can't you see from this diffstat that the patch actually seems to makes
> the code more complex?

I find that the repeated usage of a bit more error handling code is almost
unavoidable if you would like to handle allocation failures more directly
as I dared to propose again here.


> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
>     virtio_blk: Fix a slient kernel panic
> 
> which did the opposite of your patch.

This software update adjusted also the jump targets. This approach
triggered another update suggestion (in addition to improvements around
the function "kmalloc_array").

Such a software development shows different views on the implementation
for correct exception handling. I am not so "silent" on this development topic
for years.


> And in fact it fixed a bug.

I get the impression from Minfei's contribution that the statement "err = -ENOMEM;"
was added behind memory allocations.
It was also chosen to restructure this function implementation so that
the single label "out" was used there for a while.

* Is this detail worth for another look?

* How does this name selection fit to the current Linux coding style convention?


> Quite obviously multiple labels are harder to read

I do not agree agree completely to your opinion.


> and harder to get right.

These identifiers can generate their own kind of software development
challenges as usual.


> For error handling with just kfree one label is just the right thing to.

This approach can look convenient at first glance.
Does the correctness aspect need any further considerations?

Regards,
Markus

Re: virtio_blk: Less function calls in init_vq() after error detection

[SF Markus Elfring]

> In addition, please have a look at commit 347a529398e8e723338cca5d8a8ae2d9e7e93448
>     virtio_blk: Fix a slient kernel panic

I would like to add another view on the implementation details in this software update.


> which did the opposite of your patch.

This update contained a different approach for error detection and corresponding
exception handling.


> And in fact it fixed a bug.

This is great in principle according to an information in the commit description.

"…
To fix this bug, we should take care of allocation failure,
and return correct value to let caller know what happen.
…"


> Quite obviously multiple labels are harder to read and harder to get right.
> For error handling with just kfree one label is just the right thing to.

Unfortunately, I get an other impression here after a closer look.

Can it be that the discussed commit from 2016-08-09 accepted (or tolerated)
two weaknesses at least?

1. Commit title:
   Is the word "slient" a typo?
   Would you like to read "silent" there instead?

2. Source code:
   Why would another memory allocation be attempted if it could be determined quicker
   that a previous one failed and this function implementation can not succeed then?

   How much will it matter in general that two function calls are performed
   in this use case without checking their return values immediately?
   https://cwe.mitre.org/data/definitions/252.html

	if (!names || !callbacks || !vqs) { …

   https://cwe.mitre.org/data/definitions/754.html


Was the software development attention a bit too low as it happens occasionally?


I hope that my suggestions can improve the affected situation a bit more
also for this software module.

Regards,
Markus

Re: virtio_blk: Less function calls in init_vq() after error detection

[Christian Borntraeger]

On 09/13/2016 07:30 PM, SF Markus Elfring wrote:
[...]
> Unfortunately, I get an other impression here after a closer look.
> 
> Can it be that the discussed commit from 2016-08-09 accepted (or tolerated)
> two weaknesses at least?
> 
> 1. Commit title:
>    Is the word "slient" a typo?
>    Would you like to read "silent" there instead?
> 
> 2. Source code:
>    Why would another memory allocation be attempted if it could be determined quicker
>    that a previous one failed and this function implementation can not succeed then?
> 
>    How much will it matter in general that two function calls are performed
>    in this use case without checking their return values immediately?
>    https://cwe.mitre.org/data/definitions/252.html
> 
> 	if (!names || !callbacks || !vqs) { …
> 
>    https://cwe.mitre.org/data/definitions/754.html
> 

The return values are checked, just a bit later.
Markus, kernel patches are not about be formally correct vs. CodingStyle and/or checkpatch
or against code guidelines from sombody else. You will find many people consider gotos
an no-go, still it is accepted in the kernel for good reasons.
You have to think about each change and its tradeoffs (e.g simplicity vs. performance) 
for each code part again. Here we have slow path error handling, so given the low coverage
of these code parts, simplicity is important.
Yes, your code makes an unlikely error case bail out faster, but is the cost of your
patch (review time, danger of adding bugs, danger of merge conflicts, making git blame less
useful) in sync with the expected win? This is certainly Michaels area of maintainership 
and if he wants your patch, it will be fine too. (Well, having a label between the if and 
the function like
>   	if (err)
> + free_vblk_vqs:
>  		kfree(vblk->vqs);

is certainly ugly in itself)


> Was the software development attention a bit too low as it happens occasionally?
> 
> 
> I hope that my suggestions can improve the affected situation a bit more
> also for this software module.

Do you realize that your discussion style is not very helpful?
I just grepped the last LKML mails and you already pissed of several maintainers
in totally different areas. When that happens, why don't you stop for a moment and
think about "what is going wrong right now". 

Your attitude seems to be "I spend my spare time doing this, please thank me for that".
The thing is, with each patch you actually request time from the maintainer.
Now here begins the interesting part: Is the patch just a cosmetic change that does
not give you any benefit or is the patch improving the code. And remember: there
are always tradeoffs: performance, code size, code maintainability etc.

See, some of your patches are accepted, e.g. the memdup_user changes have usually
been applied by most maintainers including myself. If maintainers won't take other change,
please accept that. If you continue to waste peoples time by discussing "maybe" patches
you actually risk that people will stop taking any patches from you including the "yes"
ones.

Christian

Re: virtio_blk: Less function calls in init_vq() after error detection

[SF Markus Elfring]

>>    How much will it matter in general that two function calls are performed
>>    in this use case without checking their return values immediately?
>>    https://cwe.mitre.org/data/definitions/252.html
>>
>> 	if (!names || !callbacks || !vqs) { …
>>
>>    https://cwe.mitre.org/data/definitions/754.html
> 
> The return values are checked,

I agree to this information.


> just a bit later.

I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.


> Markus, kernel patches are not about be formally correct vs. CodingStyle and/or checkpatch

I guess that some of the involved technical advices will also matter here, don't they?


> or against code guidelines from sombody else.

Some ideas or advices are integrated from other information sources also into various
Linux software.


> You will find many people consider gotos an no-go,

I became trained also in this design direction for a while.


> still it is accepted in the kernel for good reasons.

I can follow such reasons to some degree.


> You have to think about each change and its tradeoffs (e.g simplicity vs. performance) 
> for each code part again.

This can happen as usual for an ordinary source code review process.
I would be interested to clarify if items could be optimised for repeated checks
in this work flow.


> Here we have slow path error handling, so given the low coverage of these code parts,

Would you like to add any other background information for this aspect?


> simplicity is important.

I can follow this opinion to some degree.


> Yes, your code makes an unlikely error case bail out faster,

Is this kind of functionality desired finally?


> but is the cost of your patch (review time, danger of adding bugs, danger of merge conflicts,
> making git blame less useful) in sync with the expected win?

I hope so.

Do the mentioned aspects express a fear or other general concerns which hinder changes
and make the proposed software improvement unlikely?


> (Well, having a label between the if and the function like
>>   	if (err)
>> + free_vblk_vqs:
>>  		kfree(vblk->vqs);
> 
> is certainly ugly in itself)

Would you find such a source code approach better if curly brackets will be
added there?


> Do you realize that your discussion style is not very helpful?

I find that this view is debatable.


> I just grepped the last LKML mails and you already pissed of several maintainers
> in totally different areas.

Yes. - It can happen that some contributors get occasionally grumpy.

Would you like to discuss their reasons a bit more?


> When that happens, why don't you stop for a moment
> and think about "what is going wrong right now". 

This happened also a few times already.


> Your attitude seems to be "I spend my spare time doing this, please thank me for that".

I guess that I am not so different in this aspect as I am trying to be another
respectable free software developer for years.


> The thing is, with each patch you actually request time from the maintainer.

This is a consequence of software development as usual.


> Now here begins the interesting part: Is the patch just a cosmetic change that does
> not give you any benefit or is the patch improving the code.

How do you categorise my concrete update suggestion which builds on previous contributions
by others?


> And remember: there are always tradeoffs: performance, code size, code maintainability etc.

I find that I pick some of such design goals up for my software contributions already
for a while.


> See, some of your patches are accepted, e.g. the memdup_user changes have usually
> been applied by most maintainers including myself.

I thank you once more that you could also accept one of the proposed special software updates.


> If maintainers won't take other change, please accept that.

It can happen that change acceptance needs to evolve over time.
Can a healthy pressure help to achieve special software improvements?


> If you continue to waste peoples time by discussing "maybe" patches

Can it be that this category of software updates has got a high potential for further
clarifications because some approaches are occasionally interpreted as controversial?


> you actually risk that people will stop taking any patches from you
> including the "yes" ones.

I assume that this risk is hard to avoid. - It is a matter for each contributor
on how the desired communication should evolve further.


My knowledge evolved in the way that I am using some tools for
static source code analysis. Such advanced tools can point various
change opportunities out. I picked a few special search patterns up.
It happened then that hundreds of source files were found which contain
update candidates.


Further challenges are relevant then as usual.

* Handling of the search process and their results

* Communication between contributors


Search patterns can occasionally be categorised as "too special".
The software technology contains also the risk for showing "false positives".

The reactions of code reviewers are varying between agreement and rejection.


The discussed concrete (and small) patch series is just another example
for usual difficulties or more interesting software development challenges.
I hope that they can be resolved in a systematic way.

So there are further constraints to consider, aren't there?

Regards,
Markus
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: virtio_blk: Less function calls in init_vq() after error detection

[Cornelia Huck]

On Tue, 13 Sep 2016 20:24:58 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> See, some of your patches are accepted, e.g. the memdup_user changes have usually
> been applied by most maintainers including myself. If maintainers won't take other change,
> please accept that. If you continue to waste peoples time by discussing "maybe" patches
> you actually risk that people will stop taking any patches from you including the "yes"
> ones.

FWIW, he already gained a place on my ignore list for pestering me
offline about his patches and not stopping even when told to do so. So
while I won't object if you choose to apply selected patches, I won't
comment on any and won't take any (hey, I even won't see them unless
someone else replies...)

Re: virtio_blk: Clarification for communication difficulties?

[SF Markus Elfring]

> FWIW, he already gained a place on my ignore list for pestering me
> offline about his patches and not stopping even when told to do so.

How did I "pester" you "offline"?


> So while I won't object if you choose to apply selected patches,

Another bit of interesting information, isn't it?


> I won't comment on any and won't take any (hey, I even won't see
> them unless someone else replies...)

I find this reaction strange. - I hope that our collaboration potential
can be still constructive, can't it?

Regards,
Markus

[PATCH 00/11] virtio-console: Fine-tuning for 14 function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:43:21 +0200

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

Markus Elfring (11):
  Use kmalloc_array() in init_vqs()
  Less function calls in init_vqs() after error detection
  Rename a jump label in init()
  Rename jump labels in virtcons_probe()
  Rename jump labels in add_port()
  Rename a jump label in port_fops_open()
  Rename a jump label in port_fops_splice_write()
  Rename jump labels in port_fops_write()
  Rename a jump label in __send_to_port()
  Rename jump labels in alloc_buf()
  Rename a jump label in five functions

 drivers/char/virtio_console.c | 155 ++++++++++++++++++++++++------------------
 1 file changed, 87 insertions(+), 68 deletions(-)

-- 
2.10.0

[PATCH 01/11] virtio_console: Use kmalloc_array() in init_vqs()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 11:23:59 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specifications of data types by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d2406fe..325ebc6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1881,13 +1881,17 @@ static int init_vqs(struct ports_device *portdev)
 	nr_ports = portdev->config.max_nr_ports;
 	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
 
-	vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
-	io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *), GFP_KERNEL);
-	io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
-	portdev->in_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
-				  GFP_KERNEL);
-	portdev->out_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
-				   GFP_KERNEL);
+	vqs = kmalloc_array(nr_queues, sizeof(*vqs), GFP_KERNEL);
+	io_callbacks = kmalloc_array(nr_queues,
+				     sizeof(*io_callbacks),
+				     GFP_KERNEL);
+	io_names = kmalloc_array(nr_queues, sizeof(*io_names), GFP_KERNEL);
+	portdev->in_vqs = kmalloc_array(nr_ports,
+					sizeof(*portdev->in_vqs),
+					GFP_KERNEL);
+	portdev->out_vqs = kmalloc_array(nr_ports,
+					 sizeof(*portdev->out_vqs),
+					 GFP_KERNEL);
 	if (!vqs || !io_callbacks || !io_names || !portdev->in_vqs ||
 	    !portdev->out_vqs) {
 		err = -ENOMEM;
-- 
2.10.0

[PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 14:00:35 +0200

The kfree() function was called in up to five cases
by the init_vqs() function during error handling even if
the passed variable contained a null pointer.

* Return directly after a call of the function "kmalloc_array" failed
  at the beginning.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 325ebc6..bf0ad57 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1882,20 +1882,37 @@ static int init_vqs(struct ports_device *portdev)
 	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
 
 	vqs = kmalloc_array(nr_queues, sizeof(*vqs), GFP_KERNEL);
+	if (!vqs)
+		return -ENOMEM;
+
 	io_callbacks = kmalloc_array(nr_queues,
 				     sizeof(*io_callbacks),
 				     GFP_KERNEL);
+	if (!io_callbacks) {
+		err = -ENOMEM;
+		goto free_vqs;
+	}
+
 	io_names = kmalloc_array(nr_queues, sizeof(*io_names), GFP_KERNEL);
+	if (!io_names) {
+		err = -ENOMEM;
+		goto free_callbacks;
+	}
+
 	portdev->in_vqs = kmalloc_array(nr_ports,
 					sizeof(*portdev->in_vqs),
 					GFP_KERNEL);
+	if (!portdev->in_vqs) {
+		err = -ENOMEM;
+		goto free_names;
+	}
+
 	portdev->out_vqs = kmalloc_array(nr_ports,
 					 sizeof(*portdev->out_vqs),
 					 GFP_KERNEL);
-	if (!vqs || !io_callbacks || !io_names || !portdev->in_vqs ||
-	    !portdev->out_vqs) {
+	if (!portdev->out_vqs) {
 		err = -ENOMEM;
-		goto free;
+		goto free_in_vqs;
 	}
 
 	/*
@@ -1929,7 +1946,7 @@ static int init_vqs(struct ports_device *portdev)
 					      io_callbacks,
 					      (const char **)io_names);
 	if (err)
-		goto free;
+		goto free_out_vqs;
 
 	j = 0;
 	portdev->in_vqs[0] = vqs[0];
@@ -1950,12 +1967,15 @@ static int init_vqs(struct ports_device *portdev)
 	kfree(vqs);
 
 	return 0;
-
-free:
+ free_out_vqs:
 	kfree(portdev->out_vqs);
+ free_in_vqs:
 	kfree(portdev->in_vqs);
+ free_names:
 	kfree(io_names);
+ free_callbacks:
 	kfree(io_callbacks);
+ free_vqs:
 	kfree(vqs);
 
 	return err;
-- 
2.10.0

[PATCH 03/11] virtio_console: Rename a jump label in init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 14:10:24 +0200

Adjust jump labels according to the current Linux coding style convention.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index bf0ad57..004314e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2309,7 +2309,7 @@ static int __init init(void)
 	err = register_virtio_driver(&virtio_console);
 	if (err < 0) {
 		pr_err("Error %d registering virtio driver\n", err);
-		goto free;
+		goto remove;
 	}
 	err = register_virtio_driver(&virtio_rproc_serial);
 	if (err < 0) {
@@ -2318,9 +2318,9 @@ static int __init init(void)
 		goto unregister;
 	}
 	return 0;
-unregister:
+ unregister:
 	unregister_virtio_driver(&virtio_console);
-free:
+ remove:
 	debugfs_remove_recursive(pdrvdata.debugfs_dir);
 	class_destroy(pdrvdata.class);
 	return err;
-- 
2.10.0

[PATCH 04/11] virtio_console: Rename jump labels in virtcons_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 14:24:05 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 004314e..768bbb7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2037,7 +2037,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 	portdev = kmalloc(sizeof(*portdev), GFP_KERNEL);
 	if (!portdev) {
 		err = -ENOMEM;
-		goto fail;
+		goto exit;
 	}
 
 	/* Attach this portdev to this virtio_device, and vice-versa. */
@@ -2051,7 +2051,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 			"Error %d registering chrdev for device %u\n",
 			portdev->chr_major, vdev->index);
 		err = portdev->chr_major;
-		goto free;
+		goto free_port;
 	}
 
 	multiport = false;
@@ -2068,7 +2068,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 	err = init_vqs(portdev);
 	if (err < 0) {
 		dev_err(&vdev->dev, "Error %d initializing vqs\n", err);
-		goto free_chrdev;
+		goto unregister;
 	}
 
 	spin_lock_init(&portdev->ports_lock);
@@ -2091,7 +2091,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 			dev_err(&vdev->dev,
 				"Error allocating buffers for control queue\n");
 			err = -ENOMEM;
-			goto free_vqs;
+			goto send_control_message;
 		}
 	} else {
 		/*
@@ -2121,17 +2121,16 @@ static int virtcons_probe(struct virtio_device *vdev)
 		wait_for_completion(&early_console_added);
 
 	return 0;
-
-free_vqs:
+ send_control_message:
 	/* The host might want to notify mgmt sw about device add failure */
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 0);
 	remove_vqs(portdev);
-free_chrdev:
+ unregister:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
-free:
+ free_port:
 	kfree(portdev);
-fail:
+ exit:
 	return err;
 }
 
-- 
2.10.0

[PATCH 05/11] virtio_console: Rename jump labels in add_port()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 14:53:00 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 768bbb7..40b8775 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1399,7 +1399,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	port = kmalloc(sizeof(*port), GFP_KERNEL);
 	if (!port) {
 		err = -ENOMEM;
-		goto fail;
+		goto send_control_message;
 	}
 	kref_init(&port->kref);
 
@@ -1434,7 +1434,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	if (err < 0) {
 		dev_err(&port->portdev->vdev->dev,
 			"Error %d adding cdev for port %u\n", err, id);
-		goto free_cdev;
+		goto delete_cdev;
 	}
 	port->dev = device_create(pdrvdata.class, &port->portdev->vdev->dev,
 				  devt, port, "vport%up%u",
@@ -1444,7 +1444,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 		dev_err(&port->portdev->vdev->dev,
 			"Error %d creating device for port %u\n",
 			err, id);
-		goto free_cdev;
+		goto delete_cdev;
 	}
 
 	spin_lock_init(&port->inbuf_lock);
@@ -1456,7 +1456,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	if (!nr_added_bufs) {
 		dev_err(port->dev, "Error allocating inbufs\n");
 		err = -ENOMEM;
-		goto free_device;
+		goto destroy_device;
 	}
 
 	if (is_rproc_serial(port->portdev->vdev))
@@ -1473,7 +1473,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 		 */
 		err = init_port_console(port);
 		if (err)
-			goto free_inbufs;
+			goto free_buffers;
 	}
 
 	spin_lock_irq(&portdev->ports_lock);
@@ -1500,17 +1500,16 @@ static int add_port(struct ports_device *portdev, u32 id)
 							 &port_debugfs_ops);
 	}
 	return 0;
-
-free_inbufs:
+ free_buffers:
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
-free_device:
+ destroy_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
-free_cdev:
+ delete_cdev:
 	cdev_del(port->cdev);
-free_port:
+ free_port:
 	kfree(port);
-fail:
+ send_control_message:
 	/* The host might want to notify management sw about port add failure */
 	__send_control_msg(portdev, id, VIRTIO_CONSOLE_PORT_READY, 0);
 	return err;
-- 
2.10.0

[PATCH 06/11] virtio_console: Rename a jump label in port_fops_open()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 14:58:24 +0200

Adjust a jump label according to the current Linux coding style convention.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 40b8775..99dc659 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1058,7 +1058,7 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	 */
 	if (is_console_port(port)) {
 		ret = -ENXIO;
-		goto out;
+		goto put_ref;
 	}
 
 	/* Allow only one process to open a particular port at a time */
@@ -1066,7 +1066,7 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	if (port->guest_connected) {
 		spin_unlock_irq(&port->inbuf_lock);
 		ret = -EBUSY;
-		goto out;
+		goto put_ref;
 	}
 
 	port->guest_connected = true;
@@ -1087,7 +1087,7 @@ static int port_fops_open(struct inode *inode, struct file *filp)
 	send_control_msg(filp->private_data, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
 	return 0;
-out:
+ put_ref:
 	kref_put(&port->kref, remove_port);
 	return ret;
 }
-- 
2.10.0

[PATCH 07/11] virtio_console: Rename a jump label in port_fops_splice_write()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:01:51 +0200

Adjust a jump label according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 99dc659..d8681d9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -947,17 +947,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	pipe_lock(pipe);
 	if (!pipe->nrbufs) {
 		ret = 0;
-		goto error_out;
+		goto unlock;
 	}
 
 	ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
 	if (ret < 0)
-		goto error_out;
+		goto unlock;
 
 	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
 	if (!buf) {
 		ret = -ENOMEM;
-		goto error_out;
+		goto unlock;
 	}
 
 	sgl.n = 0;
@@ -973,8 +973,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (unlikely(ret <= 0))
 		free_buf(buf, true);
 	return ret;
-
-error_out:
+ unlock:
 	pipe_unlock(pipe);
 	return ret;
 }
-- 
2.10.0

[PATCH 08/11] virtio_console: Rename jump labels in port_fops_write()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:07:42 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index d8681d9..babc812 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -842,7 +842,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	ret = copy_from_user(buf->buf, ubuf, count);
 	if (ret) {
 		ret = -EFAULT;
-		goto free_buf;
+		goto free_buffer;
 	}
 
 	/*
@@ -857,11 +857,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	ret = __send_to_port(port, sg, 1, count, buf, nonblock);
 
 	if (nonblock && ret > 0)
-		goto out;
-
-free_buf:
+		goto exit;
+ free_buffer:
 	free_buf(buf, true);
-out:
+ exit:
 	return ret;
 }
 
-- 
2.10.0

[PATCH 09/11] virtio_console: Rename a jump label in __send_to_port()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:15:06 +0200

Adjust a jump label according to the current Linux coding style convention.

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

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index babc812..69c6718 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -634,14 +634,14 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	if (err) {
 		in_count = 0;
-		goto done;
+		goto unlock;
 	}
 
 	if (out_vq->num_free == 0)
 		port->outvq_full = true;
 
 	if (nonblock)
-		goto done;
+		goto unlock;
 
 	/*
 	 * Wait till the host acknowledges it pushed out the data we
@@ -655,7 +655,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 	while (!virtqueue_get_buf(out_vq, &len)
 		&& !virtqueue_is_broken(out_vq))
 		cpu_relax();
-done:
+ unlock:
 	spin_unlock_irqrestore(&port->outvq_lock, flags);
 
 	port->stats.bytes_sent += in_count;
-- 
2.10.0

[PATCH 10/11] virtio_console: Rename jump labels in alloc_buf()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:20:30 +0200

Adjust jump labels according to the current Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 69c6718..0c4d4e7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -431,7 +431,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
 		      GFP_KERNEL);
 	if (!buf)
-		goto fail;
+		goto exit;
 
 	buf->sgpages = pages;
 	if (pages > 0) {
@@ -451,7 +451,7 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 		 * in dma-coherent.c
 		 */
 		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
-			goto free_buf;
+			goto free_buffer;
 		buf->dev = vq->vdev->dev.parent->parent;
 
 		/* Increase device refcnt to avoid freeing it */
@@ -464,15 +464,14 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 	}
 
 	if (!buf->buf)
-		goto free_buf;
+		goto free_buffer;
 	buf->len = 0;
 	buf->offset = 0;
 	buf->size = buf_size;
 	return buf;
-
-free_buf:
+ free_buffer:
 	kfree(buf);
-fail:
+ exit:
 	return NULL;
 }
 
-- 
2.10.0

[PATCH 11/11] virtio_console: Rename a jump label in five functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 14 Sep 2016 15:37:52 +0200

Adjust a jump label according to the current Linux coding style convention.
Thus replace the identifier "out" by "unlock".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/char/virtio_console.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0c4d4e7..6c90c9c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -257,11 +257,11 @@ static struct port *find_port_by_vtermno(u32 vtermno)
 	list_for_each_entry(cons, &pdrvdata.consoles, list) {
 		if (cons->vtermno == vtermno) {
 			port = container_of(cons, struct port, cons);
-			goto out;
+			goto unlock;
 		}
 	}
 	port = NULL;
-out:
+ unlock:
 	spin_unlock_irqrestore(&pdrvdata_lock, flags);
 	return port;
 }
@@ -276,11 +276,11 @@ static struct port *find_port_by_devt_in_portdev(struct ports_device *portdev,
 	list_for_each_entry(port, &portdev->ports, list) {
 		if (port->cdev->dev == dev) {
 			kref_get(&port->kref);
-			goto out;
+			goto unlock;
 		}
 	}
 	port = NULL;
-out:
+ unlock:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
 
 	return port;
@@ -296,10 +296,10 @@ static struct port *find_port_by_devt(dev_t dev)
 	list_for_each_entry(portdev, &pdrvdata.portdevs, list) {
 		port = find_port_by_devt_in_portdev(portdev, dev);
 		if (port)
-			goto out;
+			goto unlock;
 	}
 	port = NULL;
-out:
+ unlock:
 	spin_unlock_irqrestore(&pdrvdata_lock, flags);
 	return port;
 }
@@ -312,9 +312,9 @@ static struct port *find_port_by_id(struct ports_device *portdev, u32 id)
 	spin_lock_irqsave(&portdev->ports_lock, flags);
 	list_for_each_entry(port, &portdev->ports, list)
 		if (port->id == id)
-			goto out;
+			goto unlock;
 	port = NULL;
-out:
+ unlock:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
 
 	return port;
@@ -329,9 +329,9 @@ static struct port *find_port_by_vq(struct ports_device *portdev,
 	spin_lock_irqsave(&portdev->ports_lock, flags);
 	list_for_each_entry(port, &portdev->ports, list)
 		if (port->in_vq == vq || port->out_vq == vq)
-			goto out;
+			goto unlock;
 	port = NULL;
-out:
+ unlock:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
 	return port;
 }
-- 
2.10.0

Re: [PATCH 02/11] virtio_console: Less function calls in init_vqs() after error detection

[Amit Shah]

Hi,

On (Wed) 14 Sep 2016 [16:01:28], SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 14:00:35 +0200
> 
> The kfree() function was called in up to five cases
> by the init_vqs() function during error handling even if
> the passed variable contained a null pointer.
> 
> * Return directly after a call of the function "kmalloc_array" failed
>   at the beginning.
> 
> * Split a condition check for memory allocation failures so that
>   each pointer from these function calls will be checked immediately.
> 
>   See also background information:
>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>   Link: https://cwe.mitre.org/data/definitions/754.html
> 
> * Adjust jump targets according to the Linux coding style convention.

So I've seen this series and I'm not yet sure how I feel about the
patches - f.e. in this one, it adds more lines than it removes to
achieve the same effect.  I think the code is currently more readable
than after these changes.  And even if kfree is called multiple times,
it isn't a huge bother -- it's error case anyway, very unlikely to
trigger, but keeps everything very readble.


		Amit

Re: virtio_console: Less function calls in init_vqs() after error detection

[SF Markus Elfring]

>> The kfree() function was called in up to five cases
>> by the init_vqs() function during error handling even if
>> the passed variable contained a null pointer.
>>
>> * Return directly after a call of the function "kmalloc_array" failed
>>   at the beginning.
>>
>> * Split a condition check for memory allocation failures so that
>>   each pointer from these function calls will be checked immediately.
>>
>>   See also background information:
>>   Topic "CWE-754: Improper check for unusual or exceptional conditions"
>>   Link: https://cwe.mitre.org/data/definitions/754.html
>>
>> * Adjust jump targets according to the Linux coding style convention.
> 
> So I've seen this series and I'm not yet sure how I feel about the
> patches - f.e. in this one, it adds more lines than it removes to
> achieve the same effect.

I find this consequence still debatable.


> I think the code is currently more readable than after these changes.

Thanks for your constructive feedback.

Can it be that an other software development concern is eventually overlooked?


> And even if kfree is called multiple times, it isn't a huge bother

I know also that the implementation of this function tolerates the passing
of null pointers.


> -- it's error case anyway, very unlikely to trigger, but keeps everything very readble.

I suggest to reconsider this design detail if it is really acceptable
for the safe implementation of such a software module.

* How much will it matter in general that four function call were performed
  in this use case without checking their return values immediately?

* Should it usually be determined quicker if a required resource like
  memory could be acquired before trying the next allocation?

Regards,
Markus

Re: [PATCH 4/4] virtio_blk: Rename a jump label in virtblk_get_id()

[Stefan Hajnoczi]

On Tue, Sep 13, 2016 at 1:15 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 13 Sep 2016 13:50:56 +0200
>
> Adjust a jump label according to the current Linux coding style convention.

I think you mean "goto label".  "Jump label" has a different meaning,
see <linux/jump_label.h>.

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/virtio_blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 696f452..fef2bd0 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -247,10 +247,10 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
>
>         err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
>         if (err)
> -               goto out;
> +               goto put_request;
>
>         err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
> -out:
> + put_request:

I checked Documentation/CodingStyle and see no reason to rename the label.

It's also not clear why you added a space.  The CodingStyle example
does not use a space before the label.

Re: [PATCH 3/4] virtio_blk: Delete an unnecessary initialisation in init_vq()

[Stefan Hajnoczi]

On Tue, Sep 13, 2016 at 1:14 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 13 Sep 2016 13:43:50 +0200
>
> The local variable "err" will be set to an appropriate value
> by a following statement.
> Thus omit the explicit initialisation at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/virtio_blk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index d28dbcf..696f452 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -376,7 +376,7 @@ static void virtblk_config_changed(struct virtio_device *vdev)
>
>  static int init_vq(struct virtio_blk *vblk)
>  {
> -       int err = 0;
> +       int err;
>         int i;
>         vq_callback_t **callbacks;
>         const char **names;

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH 1/4] virtio_blk: Use kmalloc_array() in init_vq()

[Stefan Hajnoczi]

On Tue, Sep 13, 2016 at 1:12 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 13 Sep 2016 11:32:22 +0200
>
> Multiplications for the size determination of memory allocations
> indicated that array data structures should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/block/virtio_blk.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: virtio_blk: Clarification for communication difficulties?

[Stefan Hajnoczi]

On Wed, Sep 14, 2016 at 10:09 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> FWIW, he already gained a place on my ignore list for pestering me
>> offline about his patches and not stopping even when told to do so.
>
> How did I "pester" you "offline"?
>
>
>> So while I won't object if you choose to apply selected patches,
>
> Another bit of interesting information, isn't it?
>
>
>> I won't comment on any and won't take any (hey, I even won't see
>> them unless someone else replies...)
>
> I find this reaction strange. - I hope that our collaboration potential
> can be still constructive, can't it?

I can understand why others are fed up with this discussion.  Your
communication style is exhausting and you've pushed into the territory
where any benefits of taking the patches are not worth the time and
hassle of dealing with you.

I left Reviewed-bys on two patches.  Maybe they will get picked up.
But please think again about what Christian explained.  Reviewers and
maintainers spend time on your patches so make it worth their while.

Stefan

Re: virtio_blk: Clarification for communication difficulties?

[SF Markus Elfring]

>> I hope that our collaboration potential can be still constructive, can't it?
> 
> I can understand why others are fed up with this discussion.

Thanks for another feedback.


> Your communication style is exhausting

How did you get this impression?


> and you've pushed into the territory where any benefits of taking

Would you dare to acknowledge benefits from my update suggestions
for any other software components?


> the patches are not worth the time and hassle of dealing with you.

How are the chances that such a conclusion will change?


> I left Reviewed-bys on two patches.

Thanks for your constructive responses.


> Maybe they will get picked up.

I am also curious on how the software evolution will be continued.


> But please think again about what Christian explained.  Reviewers and
> maintainers spend time on your patches so make it worth their while.

I risk something just by proposing so many software updates for places
where change opportunities were found by special source code search patterns.

Regards,
Markus

Re: virtio_blk: Rename a jump label in virtblk_get_id()

[SF Markus Elfring]

>> Adjust a jump label according to the current Linux coding style convention.
> 
> I think you mean "goto label".  "Jump label" has a different meaning,
> see <linux/jump_label.h>.

Does a "goto" statement jump to the code position finally
which is specified by the label?


>> @@ -247,10 +247,10 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
>>
>>         err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
>>         if (err)
>> -               goto out;
>> +               goto put_request;
>>
>>         err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
>> -out:
>> + put_request:
> 
> I checked Documentation/CodingStyle and see no reason to rename the label.

Can the suggested longer identifier explain better the "what" and "why"
for this software situation?


> It's also not clear why you added a space.  The CodingStyle example
> does not use a space before the label.

Do you find information from a commit like "docs: Remove space-before-label guidance
from CodingStyle" (from 2016-09-21) interesting?   ;-)
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=79c70c304b0b443429b2a0019518532c5162817a

Regards,
Markus

Re: [PATCH 4/4] virtio_blk: Rename a jump label in virtblk_get_id()

[Michael S. Tsirkin]

On Tue, Sep 13, 2016 at 02:15:20PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 13 Sep 2016 13:50:56 +0200
> 
> Adjust a jump label according to the current Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Please don't send me such patches, I'm not really
interested in renaming labels unless it clearly adds
to a significant improvement in readability.

> ---
>  drivers/block/virtio_blk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 696f452..fef2bd0 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -247,10 +247,10 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
>  
>  	err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
>  	if (err)
> -		goto out;
> +		goto put_request;
>  
>  	err = blk_execute_rq(vblk->disk->queue, vblk->disk, req, false);
> -out:
> + put_request:
>  	blk_put_request(req);
>  	return err;
>  }
> -- 
> 2.10.0

Re: virtio_blk: Rename a jump label in virtblk_get_id()

[SF Markus Elfring]

> Please don't send me such patches, I'm not really interested in renaming labels

Thanks for your feedback.


> unless it clearly adds to a significant improvement in readability.

Have you got any more concrete conditions for the selection of appropriate
identifiers in this use case according to your software development view?

Regards,
Markus

Re: [PATCH 00/11] virtio-console: Fine-tuning for 14 function implementations

[SF Markus Elfring]

> Date: Wed, 14 Sep 2016 15:43:21 +0200
> 
> Several update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (11):
>   Use kmalloc_array() in init_vqs()
>   Less function calls in init_vqs() after error detection
>   Rename a jump label in init()
>   Rename jump labels in virtcons_probe()
>   Rename jump labels in add_port()
>   Rename a jump label in port_fops_open()
>   Rename a jump label in port_fops_splice_write()
>   Rename jump labels in port_fops_write()
>   Rename a jump label in __send_to_port()
>   Rename jump labels in alloc_buf()
>   Rename a jump label in five functions
> 
>  drivers/char/virtio_console.c | 155 ++++++++++++++++++++++++------------------
>  1 file changed, 87 insertions(+), 68 deletions(-)

Would you like to take another look at change possibilities
for this software module?

Regards,
Markus