* [PATCH] add_partition silently ignored errors @ 2007-10-29 12:22 Dirk Hohndel 2007-10-29 13:06 ` Cornelia Huck 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-10-29 12:22 UTC (permalink / raw) To: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel Yet another issue where we ignore errors - this needs someone to make sure that I am passing around the right error codes (and am cleaning up correctly) [PATCH] add_partition silently ignored errors Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> --- block/ioctl.c | 5 ++++- fs/partitions/check.c | 26 ++++++++++++++++++++------ include/linux/genhd.h | 2 +- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/block/ioctl.c b/block/ioctl.c index 52d6385..bb3933e 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user } } /* all seems OK */ - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); + if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) { + mutex_unlock(&bdev->bd_mutex); + return -EBUSY; + } mutex_unlock(&bdev->bd_mutex); return 0; case BLKPG_DEL_PARTITION: diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 722e12e..113ecc0 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part) kobject_put(&p->kobj); } -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) { struct hd_struct *p; p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) - return; + return -1; p->start_sect = start; p->nr_sects = len; @@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, p->kobj.parent = &disk->kobj; p->kobj.ktype = &ktype_part; kobject_init(&p->kobj); - kobject_add(&p->kobj); + if (kobject_add(&p->kobj)) + return -1; if (!disk->part_uevent_suppress) kobject_uevent(&p->kobj, KOBJ_ADD); - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) { + kobject_del(&p->kobj); + kfree(p); + return -1; + } if (flags & ADDPART_FLAG_WHOLEDISK) { static struct attribute addpartattr = { .name = "whole_disk", .mode = S_IRUSR | S_IRGRP | S_IROTH, }; - sysfs_create_file(&p->kobj, &addpartattr); + if (sysfs_create_file(&p->kobj, &addpartattr)) { + kobject_del(&p->kobj); + kfree(p); + return -1; + } } partition_sysfs_add_subdir(p); disk->part[part-1] = p; + + return 0; } static char *make_block_name(struct gendisk *disk) @@ -554,8 +565,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) if (from + size > get_capacity(disk)) { printk(" %s: p%d exceeds device capacity\n", disk->disk_name, p); + return -EBUSY; + } + if(add_partition(disk, p, from, size, state->parts[p].flags)) { + return -EBUSY; } - add_partition(disk, p, from, size, state->parts[p].flags); #ifdef CONFIG_BLK_DEV_MD if (state->parts[p].flags & ADDPART_FLAG_RAID) md_autodetect_dev(bdev->bd_dev+p); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index a47b802..ae6af2c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -414,7 +414,7 @@ struct unixware_disklabel { char *disk_name (struct gendisk *hd, int part, char *buf); extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern void add_partition(struct gendisk *, int, sector_t, sector_t, int); +extern int add_partition(struct gendisk *, int, sector_t, sector_t, int); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -- gitgui.0.8.4.g8d863 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 12:22 [PATCH] add_partition silently ignored errors Dirk Hohndel @ 2007-10-29 13:06 ` Cornelia Huck 2007-10-29 14:24 ` Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Cornelia Huck @ 2007-10-29 13:06 UTC (permalink / raw) To: Dirk Hohndel; +Cc: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel On Mon, 29 Oct 2007 05:22:11 -0700, Dirk Hohndel <hohndel@linux.intel.com> wrote: <only commenting on the kobject part...> > @@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, > p->kobj.parent = &disk->kobj; > p->kobj.ktype = &ktype_part; > kobject_init(&p->kobj); > - kobject_add(&p->kobj); > + if (kobject_add(&p->kobj)) > + return -1; > if (!disk->part_uevent_suppress) > kobject_uevent(&p->kobj, KOBJ_ADD); > - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); > + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) { > + kobject_del(&p->kobj); > + kfree(p); > + return -1; > + } - This is missing a KOBJ_DEL uevent if you did a KOBJ_ADD uevent. - Calling kfree() after you already registered the kobject via kobject_add() is wrong, since someone else might already have obtained a reference. You must drop your reference after kobject_del() and let the release mechanism take care of it. <I think I'm having some kind of deja vu, since it seems I've already pointed that out a couple of times to different people :) > > if (flags & ADDPART_FLAG_WHOLEDISK) { > static struct attribute addpartattr = { > .name = "whole_disk", > .mode = S_IRUSR | S_IRGRP | S_IROTH, > }; > > - sysfs_create_file(&p->kobj, &addpartattr); > + if (sysfs_create_file(&p->kobj, &addpartattr)) { > + kobject_del(&p->kobj); > + kfree(p); > + return -1; > + } Same here. You should probably also delete the link you created above. > } > partition_sysfs_add_subdir(p); > disk->part[part-1] = p; > + > + return 0; > } > > static char *make_block_name(struct gendisk *disk) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 13:06 ` Cornelia Huck @ 2007-10-29 14:24 ` Dirk Hohndel 2007-10-29 14:43 ` Cornelia Huck 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-10-29 14:24 UTC (permalink / raw) To: Cornelia Huck; +Cc: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel On Mon, Oct 29, 2007 at 02:06:57PM +0100, Cornelia Huck wrote: > On Mon, 29 Oct 2007 05:22:11 -0700, > Dirk Hohndel <hohndel@linux.intel.com> wrote: > > <only commenting on the kobject part...> > > > @@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, > > p->kobj.parent = &disk->kobj; > > p->kobj.ktype = &ktype_part; > > kobject_init(&p->kobj); > > - kobject_add(&p->kobj); > > + if (kobject_add(&p->kobj)) > > + return -1; > > if (!disk->part_uevent_suppress) > > kobject_uevent(&p->kobj, KOBJ_ADD); > > - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); > > + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) { > > + kobject_del(&p->kobj); > > + kfree(p); > > + return -1; > > + } > > > - This is missing a KOBJ_DEL uevent if you did a KOBJ_ADD uevent. Completely missed that one. > - Calling kfree() after you already registered the kobject via > kobject_add() is wrong, since someone else might already have obtained > a reference. You must drop your reference after kobject_del() and let > the release mechanism take care of it. Good point. > <I think I'm having some kind of deja vu, since it seems I've already > pointed that out a couple of times to different people :) > > > > if (flags & ADDPART_FLAG_WHOLEDISK) { > > static struct attribute addpartattr = { > > .name = "whole_disk", > > .mode = S_IRUSR | S_IRGRP | S_IROTH, > > }; > > > > - sysfs_create_file(&p->kobj, &addpartattr); > > + if (sysfs_create_file(&p->kobj, &addpartattr)) { > > + kobject_del(&p->kobj); > > + kfree(p); > > + return -1; > > + } > > Same here. You should probably also delete the link you created above. Yep, fixed that as well. How about the new patch below? Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> --- block/ioctl.c | 5 ++++- fs/partitions/check.c | 28 ++++++++++++++++++++++------ include/linux/genhd.h | 2 +- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/block/ioctl.c b/block/ioctl.c index 52d6385..bb3933e 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user } } /* all seems OK */ - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); + if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) { + mutex_unlock(&bdev->bd_mutex); + return -EBUSY; + } mutex_unlock(&bdev->bd_mutex); return 0; case BLKPG_DEL_PARTITION: diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 722e12e..6bdf855 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part) kobject_put(&p->kobj); } -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) { struct hd_struct *p; p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) - return; + return -1; p->start_sect = start; p->nr_sects = len; @@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, p->kobj.parent = &disk->kobj; p->kobj.ktype = &ktype_part; kobject_init(&p->kobj); - kobject_add(&p->kobj); + if (kobject_add(&p->kobj)) + return -1; if (!disk->part_uevent_suppress) kobject_uevent(&p->kobj, KOBJ_ADD); - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) + goto out_cleanup; if (flags & ADDPART_FLAG_WHOLEDISK) { static struct attribute addpartattr = { .name = "whole_disk", .mode = S_IRUSR | S_IRGRP | S_IROTH, }; - sysfs_create_file(&p->kobj, &addpartattr); + if (sysfs_create_file(&p->kobj, &addpartattr)) { + sysfs_remove_link (&p->kobj, "subsystem"); + goto out_cleanup; + } } partition_sysfs_add_subdir(p); disk->part[part-1] = p; + + return 0; + +out_cleanup: + if (!disk->part_uevent_suppress) + kobject_uevent(&p->kobj, KOBJ_REMOVE); + kobject_del(&p->kobj); + return -1; } static char *make_block_name(struct gendisk *disk) @@ -554,8 +567,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) if (from + size > get_capacity(disk)) { printk(" %s: p%d exceeds device capacity\n", disk->disk_name, p); + return -EBUSY; + } + if(add_partition(disk, p, from, size, state->parts[p].flags)) { + return -EBUSY; } - add_partition(disk, p, from, size, state->parts[p].flags); #ifdef CONFIG_BLK_DEV_MD if (state->parts[p].flags & ADDPART_FLAG_RAID) md_autodetect_dev(bdev->bd_dev+p); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index a47b802..ae6af2c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -414,7 +414,7 @@ struct unixware_disklabel { char *disk_name (struct gendisk *hd, int part, char *buf); extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern void add_partition(struct gendisk *, int, sector_t, sector_t, int); +extern int add_partition(struct gendisk *, int, sector_t, sector_t, int); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -- gitgui.0.8.4.g8d863 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 14:24 ` Dirk Hohndel @ 2007-10-29 14:43 ` Cornelia Huck 2007-10-29 15:48 ` Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Cornelia Huck @ 2007-10-29 14:43 UTC (permalink / raw) To: Dirk Hohndel; +Cc: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel On Mon, 29 Oct 2007 07:24:27 -0700, Dirk Hohndel <hohndel@linux.intel.com> wrote: > @@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, > p->kobj.parent = &disk->kobj; > p->kobj.ktype = &ktype_part; > kobject_init(&p->kobj); > - kobject_add(&p->kobj); > + if (kobject_add(&p->kobj)) Hm, here the structure needs to be freed as well (via kobject_put()). > + return -1; > if (!disk->part_uevent_suppress) > kobject_uevent(&p->kobj, KOBJ_ADD); > - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); > + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) Whitespace (if (...)). > + goto out_cleanup; > if (flags & ADDPART_FLAG_WHOLEDISK) { > static struct attribute addpartattr = { > .name = "whole_disk", > .mode = S_IRUSR | S_IRGRP | S_IROTH, > }; > > - sysfs_create_file(&p->kobj, &addpartattr); > + if (sysfs_create_file(&p->kobj, &addpartattr)) { > + sysfs_remove_link (&p->kobj, "subsystem"); > + goto out_cleanup; > + } > } > partition_sysfs_add_subdir(p); > disk->part[part-1] = p; > + > + return 0; > + > +out_cleanup: > + if (!disk->part_uevent_suppress) > + kobject_uevent(&p->kobj, KOBJ_REMOVE); > + kobject_del(&p->kobj); You need a kobject_put() here to drop the reference you obtained in kobject_init(). > + return -1; > } > > static char *make_block_name(struct gendisk *disk) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 14:43 ` Cornelia Huck @ 2007-10-29 15:48 ` Dirk Hohndel 2007-10-29 16:47 ` Cornelia Huck 2007-10-30 8:07 ` Jens Axboe 0 siblings, 2 replies; 18+ messages in thread From: Dirk Hohndel @ 2007-10-29 15:48 UTC (permalink / raw) To: Cornelia Huck; +Cc: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel On Mon, Oct 29, 2007 at 03:43:39PM +0100, Cornelia Huck wrote: > > > @@ -390,20 +390,33 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, > > p->kobj.parent = &disk->kobj; > > p->kobj.ktype = &ktype_part; > > kobject_init(&p->kobj); > > - kobject_add(&p->kobj); > > + if (kobject_add(&p->kobj)) > > Hm, here the structure needs to be freed as well (via kobject_put()). done > > kobject_uevent(&p->kobj, KOBJ_ADD); > > - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); > > + if(sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) > > Whitespace (if (...)). oops. > > + > > +out_cleanup: > > + if (!disk->part_uevent_suppress) > > + kobject_uevent(&p->kobj, KOBJ_REMOVE); > > + kobject_del(&p->kobj); > > You need a kobject_put() here to drop the reference you obtained in > kobject_init(). done We should be getting closer - thanks for all the help getting this right! /D [PATCH] add_partition silently ignored errors Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> --- block/ioctl.c | 5 ++++- fs/partitions/check.c | 30 ++++++++++++++++++++++++------ include/linux/genhd.h | 2 +- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/block/ioctl.c b/block/ioctl.c index 52d6385..bb3933e 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user } } /* all seems OK */ - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); + if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) { + mutex_unlock(&bdev->bd_mutex); + return -EBUSY; + } mutex_unlock(&bdev->bd_mutex); return 0; case BLKPG_DEL_PARTITION: diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 722e12e..cd92471 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part) kobject_put(&p->kobj); } -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) { struct hd_struct *p; p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) - return; + return -1; p->start_sect = start; p->nr_sects = len; @@ -390,20 +390,35 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, p->kobj.parent = &disk->kobj; p->kobj.ktype = &ktype_part; kobject_init(&p->kobj); - kobject_add(&p->kobj); + if (kobject_add(&p->kobj)) + goto out_put; if (!disk->part_uevent_suppress) kobject_uevent(&p->kobj, KOBJ_ADD); - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + if (sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem")) + goto out_cleanup; if (flags & ADDPART_FLAG_WHOLEDISK) { static struct attribute addpartattr = { .name = "whole_disk", .mode = S_IRUSR | S_IRGRP | S_IROTH, }; - sysfs_create_file(&p->kobj, &addpartattr); + if (sysfs_create_file(&p->kobj, &addpartattr)) { + sysfs_remove_link (&p->kobj, "subsystem"); + goto out_cleanup; + } } partition_sysfs_add_subdir(p); disk->part[part-1] = p; + + return 0; + +out_cleanup: + if (!disk->part_uevent_suppress) + kobject_uevent(&p->kobj, KOBJ_REMOVE); + kobject_del(&p->kobj); +out_put: + kobject_put(&p->kobj); + return -1; } static char *make_block_name(struct gendisk *disk) @@ -554,8 +569,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) if (from + size > get_capacity(disk)) { printk(" %s: p%d exceeds device capacity\n", disk->disk_name, p); + return -EBUSY; + } + if(add_partition(disk, p, from, size, state->parts[p].flags)) { + return -EBUSY; } - add_partition(disk, p, from, size, state->parts[p].flags); #ifdef CONFIG_BLK_DEV_MD if (state->parts[p].flags & ADDPART_FLAG_RAID) md_autodetect_dev(bdev->bd_dev+p); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index a47b802..ae6af2c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -414,7 +414,7 @@ struct unixware_disklabel { char *disk_name (struct gendisk *hd, int part, char *buf); extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern void add_partition(struct gendisk *, int, sector_t, sector_t, int); +extern int add_partition(struct gendisk *, int, sector_t, sector_t, int); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -- gitgui.0.8.4.g8d863 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 15:48 ` Dirk Hohndel @ 2007-10-29 16:47 ` Cornelia Huck 2007-10-30 8:07 ` Jens Axboe 1 sibling, 0 replies; 18+ messages in thread From: Cornelia Huck @ 2007-10-29 16:47 UTC (permalink / raw) To: Dirk Hohndel; +Cc: Andries Brouwer, Al Viro, Jens Axboe, linux-kernel On Mon, 29 Oct 2007 08:48:49 -0700, Dirk Hohndel <hohndel@linux.intel.com> wrote: > [PATCH] add_partition silently ignored errors > > Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> > > --- > block/ioctl.c | 5 ++++- > fs/partitions/check.c | 30 ++++++++++++++++++++++++------ > include/linux/genhd.h | 2 +- > 3 files changed, 29 insertions(+), 8 deletions(-) The kobject stuff looks sane to me now. Others will have to comment on the return code propagation. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-29 15:48 ` Dirk Hohndel 2007-10-29 16:47 ` Cornelia Huck @ 2007-10-30 8:07 ` Jens Axboe 2007-10-30 9:09 ` Cornelia Huck 1 sibling, 1 reply; 18+ messages in thread From: Jens Axboe @ 2007-10-30 8:07 UTC (permalink / raw) To: Dirk Hohndel; +Cc: Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel On Mon, Oct 29 2007, Dirk Hohndel wrote: > diff --git a/block/ioctl.c b/block/ioctl.c > index 52d6385..bb3933e 100644 > --- a/block/ioctl.c > +++ b/block/ioctl.c > @@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user > } > } > /* all seems OK */ > - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); > + if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) { > + mutex_unlock(&bdev->bd_mutex); > + return -EBUSY; > + } > mutex_unlock(&bdev->bd_mutex); > return 0; > case BLKPG_DEL_PARTITION: > diff --git a/fs/partitions/check.c b/fs/partitions/check.c > index 722e12e..cd92471 100644 > --- a/fs/partitions/check.c > +++ b/fs/partitions/check.c > @@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part) > kobject_put(&p->kobj); > } > > -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > { > struct hd_struct *p; > > p = kzalloc(sizeof(*p), GFP_KERNEL); > if (!p) > - return; > + return -1; Why not return the 'correct' error codes, instead of always -1 and making that -EBUSY at the caller? This one should be -ENOMEM. IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with the correct return values, the patch then looks fine to me. -- Jens Axboe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 8:07 ` Jens Axboe @ 2007-10-30 9:09 ` Cornelia Huck 2007-10-30 16:56 ` Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Cornelia Huck @ 2007-10-30 9:09 UTC (permalink / raw) To: Jens Axboe; +Cc: Dirk Hohndel, Andries Brouwer, Al Viro, linux-kernel On Tue, 30 Oct 2007 09:07:42 +0100, Jens Axboe <jens.axboe@oracle.com> wrote: > On Mon, Oct 29 2007, Dirk Hohndel wrote: > > diff --git a/block/ioctl.c b/block/ioctl.c > > index 52d6385..bb3933e 100644 > > --- a/block/ioctl.c > > +++ b/block/ioctl.c > > @@ -61,7 +61,10 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user > > } > > } > > /* all seems OK */ > > - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); > > + if (add_partition(disk, part, start, length, ADDPART_FLAG_NONE)) { > > + mutex_unlock(&bdev->bd_mutex); > > + return -EBUSY; > > + } > > mutex_unlock(&bdev->bd_mutex); > > return 0; > > case BLKPG_DEL_PARTITION: > > diff --git a/fs/partitions/check.c b/fs/partitions/check.c > > index 722e12e..cd92471 100644 > > --- a/fs/partitions/check.c > > +++ b/fs/partitions/check.c > > @@ -368,13 +368,13 @@ void delete_partition(struct gendisk *disk, int part) > > kobject_put(&p->kobj); > > } > > > > -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > > +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > > { > > struct hd_struct *p; > > > > p = kzalloc(sizeof(*p), GFP_KERNEL); > > if (!p) > > - return; > > + return -1; > > Why not return the 'correct' error codes, instead of always -1 and > making that -EBUSY at the caller? This one should be -ENOMEM. Oops, you're right. I agree. > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > the correct return values, the patch then looks fine to me. We need some kind of check concerning the kobject to avoid mysterious errors (especially checking for the failed kobject_add() is needed). Whether we want just to inform the user of the failure instead of failing the function is another question. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 9:09 ` Cornelia Huck @ 2007-10-30 16:56 ` Dirk Hohndel 2007-10-30 17:31 ` Cornelia Huck 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-10-30 16:56 UTC (permalink / raw) To: Cornelia Huck; +Cc: Jens Axboe, Andries Brouwer, Al Viro, linux-kernel On Tue, Oct 30, 2007 at 10:09:34AM +0100, Cornelia Huck wrote: > On Tue, 30 Oct 2007 09:07:42 +0100, > Jens Axboe <jens.axboe@oracle.com> wrote: > > > > > > > -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > > > +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) > > > { > > > struct hd_struct *p; > > > > > > p = kzalloc(sizeof(*p), GFP_KERNEL); > > > if (!p) > > > - return; > > > + return -1; > > > > Why not return the 'correct' error codes, instead of always -1 and > > making that -EBUSY at the caller? This one should be -ENOMEM. > > Oops, you're right. I agree. Duh. That was dumb of me. > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > the correct return values, the patch then looks fine to me. > > We need some kind of check concerning the kobject to avoid mysterious > errors (especially checking for the failed kobject_add() is needed). > Whether we want just to inform the user of the failure instead of > failing the function is another question. What are you suggesting? I'd love to make the behaviour consistent everywhere (and am willing to go through things in order to make that happen), but what is the consistent behaviour that we'd want? /D ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 16:56 ` Dirk Hohndel @ 2007-10-30 17:31 ` Cornelia Huck 2007-10-30 22:56 ` Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Cornelia Huck @ 2007-10-30 17:31 UTC (permalink / raw) To: Dirk Hohndel; +Cc: Jens Axboe, Andries Brouwer, Al Viro, linux-kernel On Tue, 30 Oct 2007 09:56:08 -0700, Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > > the correct return values, the patch then looks fine to me. > > > > We need some kind of check concerning the kobject to avoid mysterious > > errors (especially checking for the failed kobject_add() is needed). > > Whether we want just to inform the user of the failure instead of > > failing the function is another question. > > What are you suggesting? I'd love to make the behaviour consistent everywhere > (and am willing to go through things in order to make that happen), but what is > the consistent behaviour that we'd want? I'd be fine with just propagating the error after cleanup (that is what for example the driver core usually does), but I don't know the surrounding code well enough for a definitive answer. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 17:31 ` Cornelia Huck @ 2007-10-30 22:56 ` Dirk Hohndel 2007-10-31 9:45 ` Cornelia Huck 2007-11-02 13:04 ` Jens Axboe 0 siblings, 2 replies; 18+ messages in thread From: Dirk Hohndel @ 2007-10-30 22:56 UTC (permalink / raw) To: Cornelia Huck Cc: Jens Axboe, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On Tue, Oct 30, 2007 at 06:31:12PM +0100, Cornelia Huck wrote: > On Tue, 30 Oct 2007 09:56:08 -0700, > Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > > > the correct return values, the patch then looks fine to me. So Al, are you ok with this one? > > > We need some kind of check concerning the kobject to avoid mysterious > > > errors (especially checking for the failed kobject_add() is needed). > > > Whether we want just to inform the user of the failure instead of > > > failing the function is another question. > > > > What are you suggesting? I'd love to make the behaviour consistent everywhere > > (and am willing to go through things in order to make that happen), but what is > > the consistent behaviour that we'd want? > > I'd be fine with just propagating the error after cleanup (that is what > for example the driver core usually does), but I don't know the > surrounding code well enough for a definitive answer. Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-) /D [FILESYSTEM] add_partition ignores errors Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> --- block/ioctl.c | 9 +++++++-- fs/partitions/check.c | 36 +++++++++++++++++++++++++++++------- include/linux/genhd.h | 2 +- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/block/ioctl.c b/block/ioctl.c index 52d6385..662ec18 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -16,7 +16,7 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user struct blkpg_partition p; long long start, length; int part; - int i; + int i, err; if (!capable(CAP_SYS_ADMIN)) return -EACCES; @@ -61,7 +61,12 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user } } /* all seems OK */ - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); + err = add_partition(disk, part, start, length, + ADDPART_FLAG_NONE) + if (err) { + mutex_unlock(&bdev->bd_mutex); + return err; + } mutex_unlock(&bdev->bd_mutex); return 0; case BLKPG_DEL_PARTITION: diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 722e12e..dea79bd 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -368,13 +368,14 @@ void delete_partition(struct gendisk *disk, int part) kobject_put(&p->kobj); } -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) { struct hd_struct *p; + int err = 0; p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) - return; + return -ENOMEM; p->start_sect = start; p->nr_sects = len; @@ -390,20 +391,38 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, p->kobj.parent = &disk->kobj; p->kobj.ktype = &ktype_part; kobject_init(&p->kobj); - kobject_add(&p->kobj); + err = kobject_add(&p->kobj); + if (err) + goto out_put; if (!disk->part_uevent_suppress) kobject_uevent(&p->kobj, KOBJ_ADD); - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + err = sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + if (err) + goto out_cleanup; if (flags & ADDPART_FLAG_WHOLEDISK) { static struct attribute addpartattr = { .name = "whole_disk", .mode = S_IRUSR | S_IRGRP | S_IROTH, }; - sysfs_create_file(&p->kobj, &addpartattr); + err = sysfs_create_file(&p->kobj, &addpartattr); + if (err) { + sysfs_remove_link(&p->kobj, "subsystem"); + goto out_cleanup; + } } partition_sysfs_add_subdir(p); disk->part[part-1] = p; + + return 0; + +out_cleanup: + if (!disk->part_uevent_suppress) + kobject_uevent(&p->kobj, KOBJ_REMOVE); + kobject_del(&p->kobj); +out_put: + kobject_put(&p->kobj); + return err; } static char *make_block_name(struct gendisk *disk) @@ -530,7 +549,7 @@ exit: int rescan_partitions(struct gendisk *disk, struct block_device *bdev) { struct parsed_partitions *state; - int p, res; + int p, res, err; if (bdev->bd_part_count) return -EBUSY; @@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) if (from + size > get_capacity(disk)) { printk(" %s: p%d exceeds device capacity\n", disk->disk_name, p); + return -EBUSY; } - add_partition(disk, p, from, size, state->parts[p].flags); + err = add_partition(disk, p, from, size, state->parts[p].flags); + if (err) + return err; #ifdef CONFIG_BLK_DEV_MD if (state->parts[p].flags & ADDPART_FLAG_RAID) md_autodetect_dev(bdev->bd_dev+p); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index a47b802..ae6af2c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -414,7 +414,7 @@ struct unixware_disklabel { char *disk_name (struct gendisk *hd, int part, char *buf); extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern void add_partition(struct gendisk *, int, sector_t, sector_t, int); +extern int add_partition(struct gendisk *, int, sector_t, sector_t, int); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -- gitgui.0.8.4.g8d863 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 22:56 ` Dirk Hohndel @ 2007-10-31 9:45 ` Cornelia Huck 2007-11-02 13:04 ` Jens Axboe 1 sibling, 0 replies; 18+ messages in thread From: Cornelia Huck @ 2007-10-31 9:45 UTC (permalink / raw) To: Dirk Hohndel Cc: Jens Axboe, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On Tue, 30 Oct 2007 15:56:35 -0700, Dirk Hohndel <hohndel@linux.intel.com> wrote: > On Tue, Oct 30, 2007 at 06:31:12PM +0100, Cornelia Huck wrote: > > On Tue, 30 Oct 2007 09:56:08 -0700, > > Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > > > > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > > > > the correct return values, the patch then looks fine to me. > > So Al, are you ok with this one? > > > > > We need some kind of check concerning the kobject to avoid mysterious > > > > errors (especially checking for the failed kobject_add() is needed). > > > > Whether we want just to inform the user of the failure instead of > > > > failing the function is another question. > > > > > > What are you suggesting? I'd love to make the behaviour consistent everywhere > > > (and am willing to go through things in order to make that happen), but what is > > > the consistent behaviour that we'd want? > > > > I'd be fine with just propagating the error after cleanup (that is what > > for example the driver core usually does), but I don't know the > > surrounding code well enough for a definitive answer. > > Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-) > > /D > > > [FILESYSTEM] add_partition ignores errors > > Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> > > --- > block/ioctl.c | 9 +++++++-- > fs/partitions/check.c | 36 +++++++++++++++++++++++++++++------- > include/linux/genhd.h | 2 +- > 3 files changed, 37 insertions(+), 10 deletions(-) OK, the kobject error handling looks fine to me. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-10-30 22:56 ` Dirk Hohndel 2007-10-31 9:45 ` Cornelia Huck @ 2007-11-02 13:04 ` Jens Axboe 2007-11-02 19:29 ` Dirk Hohndel 1 sibling, 1 reply; 18+ messages in thread From: Jens Axboe @ 2007-11-02 13:04 UTC (permalink / raw) To: Dirk Hohndel Cc: Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On Tue, Oct 30 2007, Dirk Hohndel wrote: > On Tue, Oct 30, 2007 at 06:31:12PM +0100, Cornelia Huck wrote: > > On Tue, 30 Oct 2007 09:56:08 -0700, > > Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > > > > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > > > > the correct return values, the patch then looks fine to me. > > So Al, are you ok with this one? > > > > > We need some kind of check concerning the kobject to avoid mysterious > > > > errors (especially checking for the failed kobject_add() is needed). > > > > Whether we want just to inform the user of the failure instead of > > > > failing the function is another question. > > > > > > What are you suggesting? I'd love to make the behaviour consistent everywhere > > > (and am willing to go through things in order to make that happen), but what is > > > the consistent behaviour that we'd want? > > > > I'd be fine with just propagating the error after cleanup (that is what > > for example the driver core usually does), but I don't know the > > surrounding code well enough for a definitive answer. > > Ok, I think I have it consistent now. I also ran it through checkpatch.pl :-) > > /D > > > [FILESYSTEM] add_partition ignores errors Looks good to me. One final return value note: > @@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > if (from + size > get_capacity(disk)) { > printk(" %s: p%d exceeds device capacity\n", > disk->disk_name, p); > + return -EBUSY; > } -EBUSY seems a bit confusing here, although I don't know what the best value to return would be (and it probably doesn't matter). -EOVERFLOW? -ENOSPC? -- Jens Axboe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-11-02 13:04 ` Jens Axboe @ 2007-11-02 19:29 ` Dirk Hohndel 2007-11-02 19:50 ` Bob Copeland 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-11-02 19:29 UTC (permalink / raw) To: Jens Axboe Cc: Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On Fri, Nov 02, 2007 at 02:04:39PM +0100, Jens Axboe wrote: > > > > > > > > > IIRC, Al recently vetoed a similar patch. As far as I'm concerned, with > > > > > > the correct return values, the patch then looks fine to me. > > > > So Al, are you ok with this one? Still haven't seen feedback from Al... > > [FILESYSTEM] add_partition ignores errors > > Looks good to me. One final return value note: > > > @@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > > if (from + size > get_capacity(disk)) { > > printk(" %s: p%d exceeds device capacity\n", > > disk->disk_name, p); > > + return -EBUSY; > > } > > -EBUSY seems a bit confusing here, although I don't know what the best > value to return would be (and it probably doesn't matter). -EOVERFLOW? > -ENOSPC? I was wondering about that myself - EBUSY seemed to be used in a couple of other cases where there wasn't a clear match, but I think EOVERFLOW actually might make more sense. Opinions? /D ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-11-02 19:29 ` Dirk Hohndel @ 2007-11-02 19:50 ` Bob Copeland 2007-11-02 20:29 ` Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Bob Copeland @ 2007-11-02 19:50 UTC (permalink / raw) To: Dirk Hohndel Cc: Jens Axboe, Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On 11/2/07, Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > @@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > > > if (from + size > get_capacity(disk)) { > > > printk(" %s: p%d exceeds device capacity\n", > > > disk->disk_name, p); > > > + return -EBUSY; [snip] > I was wondering about that myself - EBUSY seemed to be used in a couple of > other cases where there wasn't a clear match, but I think EOVERFLOW actually > might make more sense. Opinions? ISTR that some people wanted to keep going in this case rather than return an error, e.g. for forensic purposes... .. digging... here's a thread from last year: http://lkml.org/lkml/2006/5/11/64 -Bob ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] add_partition silently ignored errors 2007-11-02 19:50 ` Bob Copeland @ 2007-11-02 20:29 ` Dirk Hohndel 2007-11-06 20:02 ` [PATCH] FS: " Dirk Hohndel 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-11-02 20:29 UTC (permalink / raw) To: Bob Copeland Cc: Jens Axboe, Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds On Fri, Nov 02, 2007 at 03:50:29PM -0400, Bob Copeland wrote: > On 11/2/07, Dirk Hohndel <hohndel@linux.intel.com> wrote: > > > > @@ -554,8 +573,11 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) > > > > if (from + size > get_capacity(disk)) { > > > > printk(" %s: p%d exceeds device capacity\n", > > > > disk->disk_name, p); > > > > + return -EBUSY; > [snip] > > I was wondering about that myself - EBUSY seemed to be used in a couple of > > other cases where there wasn't a clear match, but I think EOVERFLOW actually > > might make more sense. Opinions? > > ISTR that some people wanted to keep going in this case rather than > return an error, e.g. for forensic purposes... > > .. digging... here's a thread from last year: > > http://lkml.org/lkml/2006/5/11/64 Thanks for finding that! I took a different approach than Andries but can appreciate the argument. I'll remove that line from my patch. /D ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] FS: add_partition silently ignored errors 2007-11-02 20:29 ` Dirk Hohndel @ 2007-11-06 20:02 ` Dirk Hohndel 2007-11-06 21:38 ` Linus Torvalds 0 siblings, 1 reply; 18+ messages in thread From: Dirk Hohndel @ 2007-11-06 20:02 UTC (permalink / raw) To: Bob Copeland, Jens Axboe, Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel, Linus Torvalds (since there was no more discussion, here's the last version that includes the requested change to not fail if the partition is larger than the disk) add_partition silently ignored errors this patch passes meaningful errorcodes back and processes them in the calling functions Signed-off-by: Dirk Hohndel <hohndel@linux.intel.com> --- block/ioctl.c | 9 +++++++-- fs/partitions/check.c | 35 ++++++++++++++++++++++++++++------- include/linux/genhd.h | 2 +- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/block/ioctl.c b/block/ioctl.c index 52d6385..662ec18 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -16,7 +16,7 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user struct blkpg_partition p; long long start, length; int part; - int i; + int i, err; if (!capable(CAP_SYS_ADMIN)) return -EACCES; @@ -61,7 +61,12 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user } } /* all seems OK */ - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); + err = add_partition(disk, part, start, length, + ADDPART_FLAG_NONE) + if (err) { + mutex_unlock(&bdev->bd_mutex); + return err; + } mutex_unlock(&bdev->bd_mutex); return 0; case BLKPG_DEL_PARTITION: diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 722e12e..f802b32 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -368,13 +368,14 @@ void delete_partition(struct gendisk *disk, int part) kobject_put(&p->kobj); } -void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) +int add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, int flags) { struct hd_struct *p; + int err = 0; p = kzalloc(sizeof(*p), GFP_KERNEL); if (!p) - return; + return -ENOMEM; p->start_sect = start; p->nr_sects = len; @@ -390,20 +391,38 @@ void add_partition(struct gendisk *disk, int part, sector_t start, sector_t len, p->kobj.parent = &disk->kobj; p->kobj.ktype = &ktype_part; kobject_init(&p->kobj); - kobject_add(&p->kobj); + err = kobject_add(&p->kobj); + if (err) + goto out_put; if (!disk->part_uevent_suppress) kobject_uevent(&p->kobj, KOBJ_ADD); - sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + err = sysfs_create_link(&p->kobj, &block_subsys.kobj, "subsystem"); + if (err) + goto out_cleanup; if (flags & ADDPART_FLAG_WHOLEDISK) { static struct attribute addpartattr = { .name = "whole_disk", .mode = S_IRUSR | S_IRGRP | S_IROTH, }; - sysfs_create_file(&p->kobj, &addpartattr); + err = sysfs_create_file(&p->kobj, &addpartattr); + if (err) { + sysfs_remove_link(&p->kobj, "subsystem"); + goto out_cleanup; + } } partition_sysfs_add_subdir(p); disk->part[part-1] = p; + + return 0; + +out_cleanup: + if (!disk->part_uevent_suppress) + kobject_uevent(&p->kobj, KOBJ_REMOVE); + kobject_del(&p->kobj); +out_put: + kobject_put(&p->kobj); + return err; } static char *make_block_name(struct gendisk *disk) @@ -530,7 +549,7 @@ exit: int rescan_partitions(struct gendisk *disk, struct block_device *bdev) { struct parsed_partitions *state; - int p, res; + int p, res, err; if (bdev->bd_part_count) return -EBUSY; @@ -555,7 +574,9 @@ int rescan_partitions(struct gendisk *disk, struct block_device *bdev) printk(" %s: p%d exceeds device capacity\n", disk->disk_name, p); } - add_partition(disk, p, from, size, state->parts[p].flags); + err = add_partition(disk, p, from, size, state->parts[p].flags); + if (err) + return err; #ifdef CONFIG_BLK_DEV_MD if (state->parts[p].flags & ADDPART_FLAG_RAID) md_autodetect_dev(bdev->bd_dev+p); diff --git a/include/linux/genhd.h b/include/linux/genhd.h index a47b802..ae6af2c 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -414,7 +414,7 @@ struct unixware_disklabel { char *disk_name (struct gendisk *hd, int part, char *buf); extern int rescan_partitions(struct gendisk *disk, struct block_device *bdev); -extern void add_partition(struct gendisk *, int, sector_t, sector_t, int); +extern int add_partition(struct gendisk *, int, sector_t, sector_t, int); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); -- gitgui.0.8.4.g8d863 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] FS: add_partition silently ignored errors 2007-11-06 20:02 ` [PATCH] FS: " Dirk Hohndel @ 2007-11-06 21:38 ` Linus Torvalds 0 siblings, 0 replies; 18+ messages in thread From: Linus Torvalds @ 2007-11-06 21:38 UTC (permalink / raw) To: Dirk Hohndel Cc: Bob Copeland, Jens Axboe, Cornelia Huck, Andries Brouwer, Al Viro, linux-kernel On Tue, 6 Nov 2007, Dirk Hohndel wrote: > > (since there was no more discussion, here's the last version that includes > the requested change to not fail if the partition is larger than the disk) I'd suggest keeping it in -mm for a while. I worry about these kinds of "trivial" changes. Quite often, some errors may be normal, and breaking out early can sometimes hurt more than it helps, in that it just makes things not even limp along. That said, with the disk/partition size check removed, this looks more palatable. > - add_partition(disk, part, start, length, ADDPART_FLAG_NONE); > + err = add_partition(disk, part, start, length, > + ADDPART_FLAG_NONE) > + if (err) { > + mutex_unlock(&bdev->bd_mutex); > + return err; > + } > mutex_unlock(&bdev->bd_mutex); > return 0; However, this is just ugly. The thing is not only apparently totally untested, since it is missing a semicolon, it should also just read err = add_partition(disk, part, start, length, ADDPART_FLAG_NONE); mutex_unlock(&bdev->bd_mutex); return err; without any unnecessary conditionals (or ugly line-breaks, for that matter). > - sysfs_create_file(&p->kobj, &addpartattr); > + err = sysfs_create_file(&p->kobj, &addpartattr); > + if (err) { > + sysfs_remove_link(&p->kobj, "subsystem"); > + goto out_cleanup; Wouldn't this be better done as if (err) goto out_unlink_cleanup; to match the rest of the error handling? Linus ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2007-11-06 21:40 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-10-29 12:22 [PATCH] add_partition silently ignored errors Dirk Hohndel 2007-10-29 13:06 ` Cornelia Huck 2007-10-29 14:24 ` Dirk Hohndel 2007-10-29 14:43 ` Cornelia Huck 2007-10-29 15:48 ` Dirk Hohndel 2007-10-29 16:47 ` Cornelia Huck 2007-10-30 8:07 ` Jens Axboe 2007-10-30 9:09 ` Cornelia Huck 2007-10-30 16:56 ` Dirk Hohndel 2007-10-30 17:31 ` Cornelia Huck 2007-10-30 22:56 ` Dirk Hohndel 2007-10-31 9:45 ` Cornelia Huck 2007-11-02 13:04 ` Jens Axboe 2007-11-02 19:29 ` Dirk Hohndel 2007-11-02 19:50 ` Bob Copeland 2007-11-02 20:29 ` Dirk Hohndel 2007-11-06 20:02 ` [PATCH] FS: " Dirk Hohndel 2007-11-06 21:38 ` Linus Torvalds
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).