archive mirror
 help / color / mirror / Atom feed
From: Julia Lawall <>
To: Markus Elfring <>
Cc:, "David Airlie" <>,
	"Bernard Zhao" <>,
	"Felix Kühling" <>,,,,,
	"Daniel Vetter" <>,
	"Alex Deucher" <>,
	"Christian König" <>
Subject: Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches
Date: Sat, 20 Jun 2020 09:37:19 +0000	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2006201126130.2918@hadrien> (raw)
In-Reply-To: <>

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

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 ->

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.


> * Will the tag “Fixes” become helpful for the commit message?
> Regards,
> Markus

  reply	other threads:[~2020-06-20  9:37 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-20  9:20 [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Markus Elfring
2020-06-20  9:37 ` Julia Lawall [this message]
2020-06-20 11:16   ` =?UTF-8?B?UmU6UmU6IFtQQVRDSCB2Ml0gZHJtL2FtZGtmZDogRml4IG1lbW9yeSBsZWFrcyBhY2NvcmRpbmcgdG8gZXJyb3IgYn Bernard
2020-06-20 11:26     ` Re:Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches Julia Lawall
2020-06-20 12:56   ` Markus Elfring
2020-06-20 16:14   ` [v2] " Markus Elfring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.22.394.2006201126130.2918@hadrien \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).