Re: [PATCH 06/15] genhd: Add return code to device_add_disk

On Wed, 08/17 10:49, Cornelia Huck wrote:

On Wed, 17 Aug 2016 15:15:06 +0800
Fam Zheng [off-list ref] wrote:

quoted

@@ -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).

Yes, I think all callers need to be reviewed before device_add_disk do the
clean up on error. For this patchset I wanted to keep the change small.

Fam