Re: [PATCH 06/15] genhd: Add return code to device_add_disk
From: Cornelia Huck <hidden>
Date: 2016-08-17 08:49:31
Also in:
linux-block, linux-nvme, lkml
On Wed, 17 Aug 2016 15:15:06 +0800 Fam Zheng [off-list ref] wrote:
quoted hunk ↗ jump to hunk
@@ -613,10 +614,8 @@ void device_add_disk(struct device *parent, struct gendisk *disk) disk->flags |= GENHD_FL_UP; retval = blk_alloc_devt(&disk->part0, &devt); - if (retval) { - WARN_ON(1); - return; - } + if (retval) + goto fail; disk_to_dev(disk)->devt = devt; /* ->major and ->first_minor aren't supposed to be@@ -625,16 +624,26 @@ void device_add_disk(struct device *parent, struct gendisk *disk) disk->major = MAJOR(devt); disk->first_minor = MINOR(devt); - disk_alloc_events(disk); + retval = disk_alloc_events(disk); + if (retval) + goto fail; /* Register BDI before referencing it from bdev */ bdi = &disk->queue->backing_dev_info; - bdi_register_owner(bdi, disk_to_dev(disk)); + retval = bdi_register_owner(bdi, disk_to_dev(disk)); + if (retval) + goto fail; - blk_register_region(disk_devt(disk), disk->minors, NULL, - exact_match, exact_lock, disk); - register_disk(parent, disk); - blk_register_queue(disk); + retval = blk_register_region(disk_devt(disk), disk->minors, NULL, + exact_match, exact_lock, disk); + if (retval) + goto fail; + retval = register_disk(parent, disk); + if (retval) + goto fail; + retval = blk_register_queue(disk); + if (retval) + goto fail; /* * Take an extra ref on queue which will be put on disk_release()@@ -644,10 +653,20 @@ void device_add_disk(struct device *parent, struct gendisk *disk) retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj, "bdi"); + if (retval) + goto fail; + + retval = disk_add_events(disk); + if (retval) + goto fail; + + retval = blk_integrity_add(disk); + if (retval) + goto fail; + return 0; +fail: WARN_ON(retval); - - disk_add_events(disk); - blk_integrity_add(disk); + return retval; }
Noticed this when trying to figure out whether the error handling in virtio_blk was correct: Shouldn't you try to cleanup/rewind so that any structures are in a sane state after failure? The caller doesn't know where device_add_disk failed, and calling del_gendisk unconditionally like virtio_blk does is probably not the right thing to do (at the very least, I don't think unregistering a device that has not been registered is likely to work).