On Wed, May 30, 2018 at 11:16:10AM +1000, NeilBrown wrote:
> "obviously" we would first fix mtk_rx_free_frags() to do the right thing
> if ring->rx_data were NULL. This is a crucial part of robust error
> handling.
> The very best error handling style is devm_*. When that is not
> practical, kfree() style, which quietly accepts NULL, is a good second
> best.
> Could it be that the various bugs you have seen could be blamed on
> resource-release function which do not handle NULL well? When these
> bugs were fixed, were they fixed by fixing the resource-release
> function (which would benefit many) or were they fixed by making the
> code more robust against a poor interface by adding more checking?
It's way way more complicated to review that sort of code.
With multi label style error handling, I can just track the most
recently allocated resource and tell from the goto if it frees what I
expected.
With one label error handling, I have to scroll down to the bottom of
the function. Then jump to mtk_rx_free_frags(). Then carefully read
through it to see if can handle a partially allocated array. Then I
have to jump back to the original code and scroll up to see if
ring->rx_data is zeroed memory and where ring->rx_buf_size gets set.
>
> >
> > Multi label error handling is self documenting so you don't need to jump
> > to the bottom of the function. You only need to track the most recently
> > allocated resource.
> >
> > foo = allocate_foo();
> > if (!foo)
> > return -ENOMEM;
> >
> > bar = allocate_bar();
> > if (!bar) {
> > ret = -ENOMEM;
> > goto err_free_foo;
> > }
>
> I despise this structure because I cannot simply add
>
> baz = allocate_baz();
> if (!baz) {
> ret = -ENOMEM;
> goto err_???;
> }
>
> in the between these two. I would need to also change the err_free_foo
> to err_free_baz.
In the common case, you only need to update one goto.
foo = allocate_foo();
if (!foo)
return -ENOMEM;
ret = allocate_new();
if (ret)
goto free_foo;
ret = -ENOMEM;
bar = allocate_bar();
if (!bar)
goto free_new; <-- changed
Which ever way you write error handling, it always needs to be updated
so this isn't worse than other styles.
> Possibly if the convention was "goto err_baz_failed" it might be
> workable. Then the failure code would get
>
> free_baz(baz);
> err_baz_failed:
>
> added to it. That would be less despicable, but not as good have
> ensuring that every free_XXX() correctly handled NULL.
When code uses "come from" style label names, the gotos look like this:
foo = kmalloc();
if (!foo)
goto foo_failed;
That's useless, because it doesn't tell what the goto does. Imagine if
functions were named after the call site. It's the same concept. I
have to scroll to the bottom to see what the goto does.
>
> >
> > This style of error handling is the most maintainable. For example, I
> > see bugs where people change a function from returning NULL to returning
> > an error pointer but the error handling was relying on that kfree(NULL)
> > is a no-op. If you only free resources which have been allocated then
> > you avoid that.
>
> You've just made an excellent case for changing kfree() to something like
>
> if (unlikely(ZERO_OR_NULL_PTR(objp) || IS_ERR(objp)))
> return;
>
> I look forward do seeing your patch :-)
>
I mentioned earlier that in the past few days I had seen four bugs
from one label style error handling.
2 were freeing uninitialized pointers (GCC didn't catch it).
2 were doing kfree(foo->bar); where foo was NULL.
1 was missing a free.
Changing kfree() wouldn't have fixed any of those problems. One of the
kfree(foo->bar) bugs was actually slightly involved and we're still
figuring out what the right fix is.
The multi label bug was that was a missing free. It was straight
forward to add a new free to right place.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
