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
