Re: [PATCH] block: add_disk: fix error handling

[Neil Brown <neilb@suse.de>]

On Sun,  5 Dec 2010 16:15:27 +0100 Richard Weinberger <richard@nod.at> wrote:

> This patch brings better error handling to the
> add_disk function.
> Now it reports success or failure.

So does that mean that all callers of add_disk should now check the return
status and possible error-out ???  Sounds a bit painful.

Could you summaries the real-life situations in which add_disk can fail?
And what the consequences of failure are?

Thanks,
NeilBrown



> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  block/genhd.c         |   34 +++++++++++++++++++++++++++-------
>  include/linux/genhd.h |    2 +-
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 5fa2b44..075e630 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -508,10 +508,8 @@ static int exact_lock(dev_t devt, void *data)
>   *
>   * This function registers the partitioning information in @disk
>   * with the kernel.
> - *
> - * FIXME: error handling
>   */
> -void add_disk(struct gendisk *disk)
> +int add_disk(struct gendisk *disk)
>  {
>  	struct backing_dev_info *bdi;
>  	dev_t devt;
> @@ -529,7 +527,7 @@ void add_disk(struct gendisk *disk)
>  	retval = blk_alloc_devt(&disk->part0, &devt);
>  	if (retval) {
>  		WARN_ON(1);
> -		return;
> +		goto out;
>  	}
>  	disk_to_dev(disk)->devt = devt;
>  
> @@ -541,16 +539,38 @@ void add_disk(struct gendisk *disk)
>  
>  	/* Register BDI before referencing it from bdev */ 
>  	bdi = &disk->queue->backing_dev_info;
> -	bdi_register_dev(bdi, disk_devt(disk));
> +	retval = bdi_register_dev(bdi, disk_devt(disk));
> +	if (retval) {
> +		WARN_ON(1);
> +		goto out_free_devt;
> +	}
>  
>  	blk_register_region(disk_devt(disk), disk->minors, NULL,
>  			    exact_match, exact_lock, disk);
>  	register_disk(disk);
> -	blk_register_queue(disk);
> +	retval = blk_register_queue(disk);
> +	if (retval) {
> +		WARN_ON(1);
> +		goto out_unregister_bdi;
> +	}
>  
>  	retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
>  				   "bdi");
> -	WARN_ON(retval);
> +	if (retval) {
> +		WARN_ON(1);
> +		goto out_unregister_queue;
> +	}
> +
> +	return 0;
> +
> +out_unregister_queue:
> +	blk_unregister_queue(disk);
> +out_unregister_bdi:
> +	bdi_unregister(bdi);
> +out_free_devt:
> +	blk_free_devt(devt);
> +out:
> +	return -1;
>  }
>  
>  EXPORT_SYMBOL(add_disk);
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 7a7b9c1..4e7ab8e 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -393,7 +393,7 @@ static inline void free_part_info(struct hd_struct *part)
>  extern void part_round_stats(int cpu, struct hd_struct *part);
>  
>  /* block/genhd.c */
> -extern void add_disk(struct gendisk *disk);
> +extern int add_disk(struct gendisk *disk);
>  extern void del_gendisk(struct gendisk *gp);
>  extern void unlink_gendisk(struct gendisk *gp);
>  extern struct gendisk *get_gendisk(dev_t dev, int *partno);