From: Julia Lawall <julia.lawall@xxxxxxxx> Date: 2020-06-20 17:37:19 To: Markus Elfring <Markus.Elfring@xxxxxx> Cc: Bernard Zhao <bernard@xxxxxxxx>,opensource.kernel@xxxxxxxx,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,kernel-janitors@xxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx,Alex Deucher <alexander.deucher@xxxxxxx>,"Christian König" <christian.koenig@xxxxxxx>,"Felix Kühling" <Felix.Kuehling@xxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,David Airlie <airlied@xxxxxxxx> Subject: Re: [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? Hi: Kmemleak detected a memory leak as below: kobject_init_and_add-> kobject_add_varg-> kobject_set_name_vargs-> kvasprintf_const-> kstrdup_const-> kstrdup-> kmalloc_track_caller-> kmalloc_slab If kobject_init_and_add is called, but kobject_put is not called in the error branch. This will be detected by kmemleak. BR//Bernard >"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 >>
