[PATCH] add_partition silently ignored errors

[PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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)

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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)

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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.

Re: [PATCH] add_partition silently ignored errors

[Jens Axboe]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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.

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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.

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Cornelia Huck]

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.

Re: [PATCH] add_partition silently ignored errors

[Jens Axboe]

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

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

Re: [PATCH] add_partition silently ignored errors

[Bob Copeland]

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

Re: [PATCH] add_partition silently ignored errors

[Dirk Hohndel]

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

[PATCH] FS: add_partition silently ignored errors

[Dirk Hohndel]

(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

Re: [PATCH] FS: add_partition silently ignored errors

[Linus Torvalds]

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