Messages in this thread | | | Date | Wed, 24 Jun 2009 20:11:54 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] Fix error handling in add_disk |
| |
On Wed, 24 Jun 2009 12:18:13 +0530 Nikanth Karthikesan <knikanth@suse.de> wrote:
> On Wednesday 24 June 2009 02:53:57 Andrew Morton wrote: > > On Wed, 17 Jun 2009 12:31:10 +0530 > > > > Nikanth Karthikesan <knikanth@suse.de> 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?
| |