On Tue, May 29 2018, Dan Carpenter wrote:
> On Mon, May 21, 2018 at 09:37:31AM +1000, NeilBrown wrote:
>> On Fri, May 18 2018, Kamal Heib wrote:
>>
>> > Simplify the code of allocate and cleanup RX ring resources by using
>> > helper functions, also make sure to free the allocated resources in
>> > cause of allocation failure.
>> >
>> > Signed-off-by: Kamal Heib <kamalheib1@xxxxxxxxx>
>> > ---
>> > drivers/staging/mt7621-eth/mtk_eth_soc.c | 122 ++++++++++++++++++++-----------
>> > 1 file changed, 81 insertions(+), 41 deletions(-)
>> >
>> ....
>> > static int mtk_dma_rx_alloc(struct mtk_eth *eth, struct mtk_rx_ring *ring)
>> > {
>> > - int i, pad = 0;
>> > + int err;
>> >
>> > ring->frag_size = mtk_max_frag_size(ETH_DATA_LEN);
>> > ring->rx_buf_size = mtk_max_buf_size(ring->frag_size);
>> > @@ -317,38 +366,23 @@ static int mtk_dma_rx_alloc(struct mtk_eth *eth, struct mtk_rx_ring *ring)
>> > ring->rx_data = kcalloc(ring->rx_ring_size, sizeof(*ring->rx_data),
>> > GFP_KERNEL);
>> > if (!ring->rx_data)
>> > - goto no_rx_mem;
>> > + return -ENOMEM;
>>
>> Hi
>> I haven't tested this patch yet (will try to in the next day or so) but
>> it mostly looks good to me.
>> I would rather see the above "return -ENOMEM" as a "goto free_rx_data".
>> Having a single error-exit is generally more maintainable than having
>> lots of separate 'return's, and kfree() is quite happy to be asked to
>> kfree(NULL).
>> In fact, all of the free function (even dma_free_coherent) cope
>> ok will not having anything to free. As devm_kzallo() and kcalloc()
>> are used, all the pointers default to NULL which is safe. I would would
>> prefer a single 'nomem' label which did:
>>
>> nomem:
>> dma_free_coherent(eth->dev, ring->rx_ring_size * sizeof(*ring->rx_dma),
>> ring->rx_dma, ring->rx_phys);
>> mtk_rx_free_frags(ring);
>> kfree(ring->rx_data);
>> return -ENOMEM;
>>
>>
>> But that is just my personal preference.
>
> You're sort of advocating two closely related styles of error handling.
> The first is a do-nothing error handling instead of direct returns. The
> other style is a do-everything style error handling.
Yes, exactly. And when there is nothing that needs doing, "everything"
and "nothing" are the same thing.
>
> Direct returns are preferable to do-nothing gotos because they are more
> readable. "return -ENOMEM;" is more clear than "ret = -ENOMEM;
> goto free_rx_data;". The second one seems clear but it doesn't free
> anything so the label name is actively misleading and confusing. Do
> nothing gotos introduce "forgot to set the error code" bugs.
I don't like direct returns because they do not make modification easy.
If I find a need to allocate something else earlier, I need to go
and find all the direct returns and turn them into gotos. I prefer to
make them gotos from the start.
And your examples are not good. My preferred pattern is:
ret = -ENOMEM;
foo = alloc_foo();
if (!foo)
goto abort;
You may only need to set ret once or twice.
I see nothing misleading or confusing there.
>
> The do-everything style error handling is the most bug prone style in
> the kernel. So far this week I have seen 4 buggy one label patches vs
> 1 buggy multi label patch.
I cannot challenge your statistics, but I wonder if you might be
throwing the baby out with the bath water here. Undoubtedly it is
possible to write do-everything error handling badly. It is also
possible to write it well. I'm not convinced it is fair blame the bad
code on the broad description "do everything error handling".
>
> I feel slightly bad picking on you but with your sample code which uses
> the nomem: label, you can't free dma resources which haven't been
> allocated. I know you would have spotted it if you were writing a real
> patch instead of just throwing together sample code in your email
> client. But the second issue is that you can't call
> mtk_rx_free_frags(ring); when "ring->rx_data" is NULL or it will Oops.
> That's subtle and so far as I know, static analysis would not warn about
> it.
"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?
>
> 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.
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.
>
> 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 :-)
>
> One argument I have heard is that using single exit style code will
> prevent locking bugs where people do a direct return without dropping
> the lock. I once looked at git history to find if this was true or not.
> My conclusion was that there are some people who add direct returns
> without thinking about locking at all. Which ever style of error
> handling you use will not affect them.
>
> Sorry for the rant... I should probably try to put together some
> figures to back this up... But I think that if you look at the number
> of bugs, multi label error handling is going to be the clear winner.
No need to apologize, I enjoyed your rant - it gave me an excuse to rant
in return ;-)
Maybe more use of IS_ERR_OR_NULL() in resource release functions would
tip the scales concerning which pattern is the clear winner.
Thanks,
NeilBrown
>
> regards,
> dan carpenter
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
