Re: [PATCH] Fix error handling in add_disk

[Andrew Morton]

On Wed, 24 Jun 2009 12:18:13 +0530 Nikanth Karthikesan <knikanth@xxxxxxx> wrote:

> On Wednesday 24 June 2009 02:53:57 Andrew Morton wrote:
> > On Wed, 17 Jun 2009 12:31:10 +0530
> >
> > Nikanth Karthikesan <knikanth@xxxxxxx> wrote:
> > > Fix error handling in add_disk. Also add WARN_ON()'s in case
> > > of error, which can be removed once the callers handle the error.
> >
> > I have a vague ancestral memory that some of the unchecked errors which
> > you're now checking for actually do happen in practice, and that this
> > "fix" will end up breaking currently-working setups.
> >
> > Or maybe I'm thinking of a similar but different piece of code (maybe
> > it was the partition code?).
> >
> > Still, I think it would be prudent to initially make this patch
> > continue to ignore the errors.  So add the warnings, but don't change
> > the response to errors.  Then we can get the change distributed for a
> > bit of testing and if that all looks good then we can add the control
> > flow changes later.
> >
> 
> add_disk and blk_register_region are functions returning void masking the
> error

ugh, we're bad.

> which this patch changes. So no caller check for it's return value! And
> hence errors are ignored, and nothing breaks.

It _does_ change behaviour.  add_disk() can now bale out if, for
example, sysfs_create_link() failed.  As it commonly does, due to
various screwups.

> > >  	retval = blk_alloc_devt(&disk->part0, &devt);
> > >  	if (retval) {
> > >  		WARN_ON(1);
> > > -		return;
> > > +		goto err_out;
> > >  	}
> > > ...
> > > +	if (retval) {
> > > +		WARN_ON(1);
> > > +		goto err_free_devt;
> > > +	}
> > > ...
> > > +	if (retval) {
> > > +		WARN_ON(1);
> > > +		goto err_free_region;
> > > +	}
> > > ...
> > > -	WARN_ON(retval);
> > > +	if (retval) {
> > > +		WARN_ON(1);
> > > +		goto err_free_queue;
> > > +	}
> >
> > These all can be coded as
> >
> > 	if (WARN_ON(retval))
> > 		goto foo;
> 
> done.
> 
> Thanks
> Nikanth
> 
> 
> Fix error handling in add_disk. Also add WARN_ON()'s in case
> of error, which can be removed once the callers handle the error.

The changelog hasn't been updated to reflect this discussion.  There's
information missing here.


Also, why was this patch written?  Have you observed some behaviour
which this patch improved or corrected?

I applaud the effort, but it's obviously incomplete.  Do you intend to
add further error checking and handling in this area?
--
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/