Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

From: Julia Lawall
Date: Tue Jan 25 2011 - 06:32:16 EST


On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 25, 2011 at 12:18:40PM +0100, Julia Lawall wrote:
> > On Tue, 25 Jan 2011, walter harms wrote:
> > > So these is a bug ? They should return -ENOENT ?
> > >
> > > The interessting question is: what to do with an error ?
> > >
> > > Obviously some architecture can live with NULL, so it is not an critical
> > > error. An the patch shows a code that is simply a return, not even the
> > > user is informed that something did not work as expected.
> > >
> > > From that point of view i would like question if it is useful to have
> > > a "detailed" error instead of just returning NULL.
> >
> > Somewhat unrelatedly, I often run into code where error handling code is
> > needed, but not present, and the function returns void, so nothing is
> > provided for propagating the error further. I generally consider these
> > cases to be beyond my expertise to fix...
>
> That is a pain, but so is returning NULL in error conditions. If you've
> got several layers of nesting, and every level returns NULL on error,
> it's an awful lot of debugging to find out _why_ a failure happened.
>
> With error codes, it narrows down the number of places which could have
> returned that error code, and as error codes can be descriptive, it
> turns it into an "oh, I forgot about doing X" or "it's failing *there*"
> rather than a puzzle.
>
> The only place where it really makes sense to return NULL is with memory
> allocators. NULL is an accepted value for meaning "I couldn't allocate
> memory" as its not a useful pointer value.
>
> The alternative is to have an API like:
>
> struct clk *clk_get(int *error, ...)
> or
> int clk_get(struct clk **, ...)
>
> but that then leads to _additional_ errors made by driver authors and by
> implementations - you can no longer guarantee that *error will always be
> initialized, and this is why the whole ERR_PTR/PTR_ERR/IS_ERR stuff was
> implemented. The kernel used to have such things in it and they were
> buggy.

I agree that error codes are very useful. The problem is rather how to
propagate any sort of error indicator, whether ERR_PTR, NULL, an negative
integer, etc.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/