Re: [Patch net 2/2] net_sched: always call ->destroy when ->init() fails

[Patrick McHardy <kaber@trash.net>]

On Fri, Oct 24, 2014 at 05:53:33PM -0700, Cong Wang wrote:
> On Fri, Oct 24, 2014 at 5:44 PM, Patrick McHardy <kaber@trash.net> wrote:
> > On Fri, Oct 24, 2014 at 05:38:48PM -0700, Cong Wang wrote:
> >> On Fri, Oct 24, 2014 at 5:36 PM, Patrick McHardy <kaber@trash.net> wrote:
> >> >
> >> > Again, the correct fix is to make qdisc_create_dflt() not call
> >> > qdisc_destroy() but clean up the qdisc manually as done in
> >> > qdisc_create().
> >>
> >> I kindly wish you a good luck with fixing all callers of qdisc_create_dflt().
> >> Go ahead. :)
> >
> > Here you go:
> >
> 
> ...
> 
> Did you check all ->init() call ->destroy() on failure? Look at the
> sch_pie I have fixed in 1/2.

Why should they? They need to clean up internally, how they do it is
entirely up to them.

> Also check those xxx_init() calling xxx_change().

Please point to conrete bugs if you have any doubts. Real ones, not things
like qdisc_watchdog_init(). This is how the API to which the qdiscs have
been written has always worked.

And yes, I did check the qdisc error paths many times in the past.

> Really, we don't have to make all ->init() doing cleanup by itself.

Are you seriously suggesting that it would be better to have ->destroy()
check what parts were actually initialized and what needs to be cleaned
up instead of assuming a consistent state and have the only function that
actually knows the current state on error (->init()) do its own cleanup?

That's not even worth arguing about, its utterly and completely wrong.