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