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.
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.
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 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.
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;
}
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.
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.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
