On Mon, May 21, 2007 at 06:30:15PM -0700, Ken Chen wrote:
> On 5/21/07, Al Viro <[EMAIL PROTECTED]> wrote:
> >On Mon, May 21, 2007 at 03:00:55PM -0700, [EMAIL PROTECTED] wrote:
> >> + if (register_blkdev(LOOP_MAJOR, "loop"))
> >> + return -EIO;
> >> + blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
> >> + THIS_MODULE, loop_probe, NULL, NULL);
> >> +
> >> + for (i = 0; i < nr; i++) {
> >> + if (!loop_init_one(i))
> >> + goto err;
> >> + }
> >> +
> >> + printk(KERN_INFO "loop: module loaded\n");
> >> + return 0;
> >> +err:
> >> + loop_exit();
> >
> >This isn't good. You *can't* fail once a single disk has been registered.
> >Anyone could've opened it by now.
> >
> >IOW, you need to
> > * register region *after* you are past the point of no return
>
> That option is a lot harder than I thought. This requires an array to
> keep intermediate result of preallocated "lo" device, blk_queue, and
> disk structure before calling add_disk() or register region. And this
> array could be potentially 1 million entries. Maybe I will use
> vmalloc for it, but seems rather sick.
No, it doesn't. Really. It's easy to split; untested incremental to your
patch follows:
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 0aae8d8..2300490 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1394,16 +1394,11 @@ int loop_unregister_transfer(int number)
EXPORT_SYMBOL(loop_register_transfer);
EXPORT_SYMBOL(loop_unregister_transfer);
-static struct loop_device *loop_init_one(int i)
+static struct loop_device *loop_alloc(int i)
{
struct loop_device *lo;
struct gendisk *disk;
- list_for_each_entry(lo, &loop_devices, lo_list) {
- if (lo->lo_number == i)
- return lo;
- }
-
lo = kzalloc(sizeof(*lo), GFP_KERNEL);
if (!lo)
goto out;
@@ -1427,8 +1422,6 @@ static struct loop_device *loop_init_one(int i)
disk->private_data = lo;
disk->queue = lo->lo_queue;
sprintf(disk->disk_name, "loop%d", i);
- add_disk(disk);
- list_add_tail(&lo->lo_list, &loop_devices);
return lo;
out_free_queue:
@@ -1439,6 +1432,23 @@ out:
return NULL;
}
+static struct loop_device *loop_init_one(int i)
+{
+ struct loop_device *lo;
+
+ list_for_each_entry(lo, &loop_devices, lo_list) {
+ if (lo->lo_number == i)
+ return lo;
+ }
+
+ lo = loop_alloc(i);
+ if (lo) {
+ add_disk(lo->lo_disk);
+ list_add_tail(&lo->lo_list, &loop_devices);
+ }
+ return lo;
+}
+
static void loop_del_one(struct loop_device *lo)
{
del_gendisk(lo->lo_disk);
@@ -1481,6 +1491,7 @@ static int __init loop_init(void)
{
int i, nr;
unsigned long range;
+ struct loop_device *lo;
/*
* loop module now has a feature to instantiate underlying device
@@ -1506,19 +1517,34 @@ static int __init loop_init(void)
if (register_blkdev(LOOP_MAJOR, "loop"))
return -EIO;
- blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
- THIS_MODULE, loop_probe, NULL, NULL);
for (i = 0; i < nr; i++) {
- if (!loop_init_one(i))
- goto err;
+ lo = loop_alloc(i);
+ if (!lo)
+ goto Enomem;
+ list_add_tail(&lo->lo_list, &loop_devices);
}
+ /* point of no return */
+
+ list_for_each_entry(lo, &loop_devices, lo_list)
+ add_disk(lo->lo_disk);
+
+ blk_register_region(MKDEV(LOOP_MAJOR, 0), range,
+ THIS_MODULE, loop_probe, NULL, NULL);
+
printk(KERN_INFO "loop: module loaded\n");
return 0;
-err:
- loop_exit();
+
+Enomem:
printk(KERN_INFO "loop: out of memory\n");
+
+ while(!list_empty(&loop_devices)) {
+ lo = list_entry(loop_devices.next, struct loop_device, lo_list);
+ loop_del_one(lo);
+ }
+
+ unregister_blkdev(LOOP_MAJOR, "loop");
return -ENOMEM;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
