Re: [PATCH] block:Add proper error checking to genhd.c for the function,add_disk

Re: [PATCH] block:Add proper error checking to genhd.c for the function,add_disk

[Jens Axboe]

On 02/09/2015 06:22 PM, Nicholas Krause wrote:
> Adds error handling to the function add_disk. Error checking is
> added by removing the calls to WARN_ON when there is a failure in
> various places in the function to proper return statements with
> a error code. Further more due to this we must also add a statement
> to return 0 at the end of the function to signal success to the
> caller of the function to notify it that the function,add_disk has
> succeeded and change the function's return type to a int now to
> allow returning of error codes  for the function,add_disk to be
> allowed.

There's a lot wrong with this still:

> @@ -595,10 +594,9 @@ void add_disk(struct gendisk *disk)
>   	disk->flags |= GENHD_FL_UP;
>
>   	retval = blk_alloc_devt(&disk->part0, &devt);
> -	if (retval) {
> -		WARN_ON(1);
> -		return;
> -	}
> +	if (retval)
> +		return -ENOMEM;

Why isn't that returning retval?

> +
>   	disk_to_dev(disk)->devt = devt;
>
>   	/* ->major and ->first_minor aren't supposed to be
> @@ -622,13 +620,16 @@ void add_disk(struct gendisk *disk)
>   	 * Take an extra ref on queue which will be put on disk_release()
>   	 * so that it sticks around as long as @disk is there.
>   	 */
> -	WARN_ON_ONCE(!blk_get_queue(disk->queue));
> +	if (!blk_get_queue(disk->queue))
> +		return -ENODEV;

You just leaked the devt alloc.

>   	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
>   				   "bdi");
> -	WARN_ON(retval);
> -
> +	if (retval)
> +		return -EFAULT;

Leaked devt alloc and queue ref.

On top of that, some of the functions that add_disk() calls can fail, 
yet it doesn't check for those. And the most major part of this is 
ensuring that all callders of add_disk() properly check and handle the 
error it will pass back, that is completely ignored. Just making 
add_disk() error and unroll itself is the smaller part of the task.


-- 
Jens Axboe