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 <[email protected]>
---
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
On Mon, 29 Oct 2007 05:22:11 -0700,
Dirk Hohndel <[email protected]> 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)
On Mon, Oct 29, 2007 at 02:06:57PM +0100, Cornelia Huck wrote:
> On Mon, 29 Oct 2007 05:22:11 -0700,
> Dirk Hohndel <[email protected]> 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 <[email protected]>
---
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
On Mon, 29 Oct 2007 07:24:27 -0700,
Dirk Hohndel <[email protected]> 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)
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 <[email protected]>
---
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
On Mon, 29 Oct 2007 08:48:49 -0700,
Dirk Hohndel <[email protected]> wrote:
> [PATCH] add_partition silently ignored errors
>
> Signed-off-by: Dirk Hohndel <[email protected]>
>
> ---
> 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.
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
On Tue, 30 Oct 2007 09:07:42 +0100,
Jens Axboe <[email protected]> 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.
On Tue, Oct 30, 2007 at 10:09:34AM +0100, Cornelia Huck wrote:
> On Tue, 30 Oct 2007 09:07:42 +0100,
> Jens Axboe <[email protected]> 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
On Tue, 30 Oct 2007 09:56:08 -0700,
Dirk Hohndel <[email protected]> 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.
On Tue, Oct 30, 2007 at 06:31:12PM +0100, Cornelia Huck wrote:
> On Tue, 30 Oct 2007 09:56:08 -0700,
> Dirk Hohndel <[email protected]> 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 <[email protected]>
---
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
On Tue, 30 Oct 2007 15:56:35 -0700,
Dirk Hohndel <[email protected]> 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 <[email protected]> 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 <[email protected]>
>
> ---
> 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.
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 <[email protected]> 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
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
On 11/2/07, Dirk Hohndel <[email protected]> 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
On Fri, Nov 02, 2007 at 03:50:29PM -0400, Bob Copeland wrote:
> On 11/2/07, Dirk Hohndel <[email protected]> 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
(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 <[email protected]>
---
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
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