lkml.org 
[lkml]   [2020]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches


    On Sat, 20 Jun 2020, Markus Elfring wrote:

    > > The function kobject_init_and_add alloc memory like:
    > > kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
    > > ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
    > > ->kmalloc_slab, in err branch this memory not free. If use
    > > kmemleak, this path maybe catched.
    > > These changes are to add kobject_put in kobject_init_and_add
    > > failed branch, fix potential memleak.
    >
    > I suggest to improve this change description.
    >
    > * Can an other wording variant be nicer?

    Markus's suggestion is as usual extremely imprecise. However, I also find
    the message quite unclear.

    It would be good to always use English words. alloc and err are not
    English words. Perhaps most people will figure out what they are
    abbreviations for, but it would be better to use a few more letters to
    make it so that no one has to guess.

    Then there are a bunch of things that are connected by arrows with no
    spaces between them. The most obvious meaning of an arrow with no space
    around it is a variable dereference. After spending some mental effort,
    one can realize that that is not what you mean here. A layout like:

    first_function ->
    second_function ->
    third_function

    would be much more readable.

    I don't know what "this patch maybe catched" means. Is "catched" supposed
    to be "caught" or "cached"? Overall, the sentence could be "Kmemleak
    could possibly detect this issue", or something like that. But I don't
    know what this means. Did you detect the problem with kmemleak? if you
    did not detect the problem with kmemleak, and overall you don't know
    whether kmemleak would detect the bug or not, is this information useful
    at all for the patch?

    "These changes are to" makes a lot of words with no information. While it
    is perhaps not necessary to slavishly follow the rule about using the
    imperative, if it is convenient to use the imperative, doing so eliminates
    such meaningless phrases.

    memleak is also not an English word. Memory leak is only a few more
    characters, and doesn't require the reader to make the small extra effort
    to figure out what you mean.

    julia

    >
    > * Will the tag “Fixes” become helpful for the commit message?
    >
    > Regards,
    > Markus
    >
    \
     
     \ /
      Last update: 2020-06-20 11:38    [W:3.057 / U:1.508 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site