On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
>
> On Tue, 17 Jun 2014, Joe Perches wrote:
>
> > (adding Jesper Juhl)
> >
> > On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > > This patch adds a trivial script warning on
> > > > >
> > > > > if(foo)
> > > > > kfree(foo)
> > > > >
> > > > > (based on checkpatch)
> > > > []
> > > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci
> > > > > b/scripts/coccinelle/free/cond_kfree.cocci
> > > > []
> > > > > +* if (E)
> > > > > +* kfree@p(E);
> > > >
> > > > You should probably add all of the unnecessary
> > > > conditional tests that checkpatch uses:
> > > >
> > > > kfree
> > > > usb_free_urb
> > > > debugfs_remove
> > > > debugfs_remove_recursive
> > >
> > > Personally, I would prefer that the message encourage the user to
> > > consider
> > > whether it is necessary to call these functions with NULL as an argument
> > > in any case.
> >
> > Jesper quite awhile ago wrote:
> >
> > https://lkml.org/lkml/2005/10/13/81
> >
> > - Since kfree always checks for a NULL argument there's no reason to have an
> > additional check prior to calling kfree. It's redundant.
> > - In many cases gcc produce significantly smaller code without the redundant
> > check before the call.
> > - It's been shown in the past (in discussions on LKML) that it's generally a
> > win performance wise to avoid the extra NULL check even though it might save
> > a function call. Only when the NULL check avoids the function call in the
> > vast
> > majority of cases and the code is in a hot path does it make sense to have
> > it.
> > - This patch removes quite a few more source lines than it adds, cutting
> > down
> > on the overall number of source lines is generally a good thing.
> > - This patch reduces the indentation level, which is nice when the kfree
> > call
> > is inside some deeply nested construct.
>
> What I don't like is:
>
> a = kmalloc(...);
> if (!a) goto out;
> b = kmalloc(...);
> if (!b) goto out;
> c = kmalloc(...);
> if (!c) goto out;
> ...
> out:
> kfree(a);
> kfree(b);
> kfree(c);
>
> With a little thought, one could reorganize the code to not call kfree on
> a null value.
And I think most kernel malloc failures are written like:
a = kmalloc(...);
if (!a) goto out1;
b = kmalloc(...);
if (!b) goto out2;
c = kmalloc(...)
if (!c) goto out3;
...
out3: kfree(b);
out2: kfree(a);
out1: ...
> Someone who is not familiar with Linux programming style
> could interpret the feedback as that the above code is perfectly fine.
> (And perhaps some people do consider that it is perfectly fine).
maybe so.
> On the other hand, in the case:
>
> x = NULL;
> if (complicated_condition)
> x = kmalloc(...);
> if (!x) return;
> y = something(...);
> if (!y)
> goto out1;
> ...
> out1: kfree(x);
>
> I guess it's OK. Mildly unpleasant, but probably the best option given
> the various tradeoff.
>
> In looking at Jesper's patch, I see that another case is:
>
> a = kmalloc(...);
> b = kmalloc(...);
> if (!a || !b) {
> kfree(a);
> kfree(b);
> }
>
> Personally, I would rather see each call have its own error handling code.
> There is no point to make the second call if the first one fails.
>
> When one tries to understand code, the main questions are why is this done
> here, and why is this not done here. Doing things that are unnecessary
> introduces confusion in this regard. Perhaps it doesn't matter for
> kmalloc and kfree because everyone is familiar with them and they are
> pretty innocuous. But for the more obscure functions, like in my
> recollection of Markus's patch, I'm not convinced that simply blindly
> removing all unneeded tests without thinking whether the code could be
> written in a better way is a good idea.
Blindly applying patches, even those produced by coccinelle,
let alone mine, is rarely good practice.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
