On Thu, Sep 12, 2019 at 2:10 AM Cong Wang <xiyou.wangc...@gmail.com> wrote:
>
> On Wed, Sep 11, 2019 at 2:36 PM Eric Dumazet <eric.duma...@gmail.com> wrote:
> >
> > It seems a similar fix would be needed in net/sched/sch_dsmark.c ?
> >
>
> Yeah, or just add a NULL check in dsmark_destroy().
Well, this was why one of my suggestions was to just make
"qdisc_put()" be happy with a NULL pointer (or even an ERR_PTR()).
That would have fixed not just sfb, but also dsmark with a single patch.
We tend to have that kind of pattern in a lot of places, where we can
free unallocated structures (end ERR_PTR() pointers) withour errors,
so that
destroy_fn(alloc_fn());
is defined to always work, even when alloc_fn() returns NULL or an
error. That, and allowing the "it was never allocated at all" case (as
long as it's initialized to NULL, of course) tends to make various
error cases simpler.
The obvious one is kfree(kmalloc()), of course, but we've done it in
other places too. So you find things like
void i2c_unregister_device(struct i2c_client *client)
{
if (IS_ERR_OR_NULL(client))
return;
in various subsystems and drivers. So one of my suggestions was to
just do that to qdisc_put().
It depends on what you want to do, of course. Do you want to make sure
each user is being very careful? Or do you want to make the interfaces
easy to use without _having_ to be careful? There are arguments both
ways, but we've tended to move more towards a "easy to use" model than
the "be careful" one.
Linus
