lkml.org 
[lkml]   [2016]   [Sep]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: virtio_blk: Less function calls in init_vq() after error detection
    From
    Date
    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

    \
     
     \ /
      Last update: 2016-09-17 09:59    [W:2.086 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site