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