Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread Dirk Hohndel 
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread Bob Copeland 
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread 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 <[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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread 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 [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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread Bob Copeland 
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread 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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-11-02 Thread Dirk Hohndel 
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-31 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-31 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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 <[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(>bd_mutex);
+   return err;
+   }
mutex_unlock(>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(>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 = >kobj;
p->kobj.ktype = _part;
kobject_init(>kobj);
-   kobject_add(>kobj);
+   err = kobject_add(>kobj);
+   if (err)
+   goto out_put;
if (!disk->part_uevent_suppress)
kobject_uevent(>kobj, KOBJ_ADD);
-   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
+   err = sysfs_create_link(>kobj, _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(>kobj, );
+   err = sysfs_create_file(>kobj, );
+   if (err) {
+   sysfs_remove_link(>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(>kobj, KOBJ_REMOVE);
+   kobject_del(>kobj);
+out_put:
+   kobject_put(>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)) {

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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 <[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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread Cornelia Huck 
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(>bd_mutex);
> > +   return -EBUSY;
> > +   }
> > mutex_unlock(>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(>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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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(>bd_mutex);
> + return -EBUSY;
> + }
>   mutex_unlock(>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(>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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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 [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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-30 Thread 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 [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))

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 = >kobj;
> > p->kobj.ktype = _part;
> > kobject_init(>kobj);
> > -   kobject_add(>kobj);
> > +   if (kobject_add(>kobj)) 
> 
> Hm, here the structure needs to be freed as well (via kobject_put()).

done

> > kobject_uevent(>kobj, KOBJ_ADD);
> > -   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
> > +   if(sysfs_create_link(>kobj, _subsys.kobj, "subsystem")) 
> 
> Whitespace (if (...)).

oops.

> > +
> > +out_cleanup:
> > +   if (!disk->part_uevent_suppress)
> > +   kobject_uevent(>kobj, KOBJ_REMOVE);
> > +   kobject_del(>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(>bd_mutex);
+   return -EBUSY;
+   }
mutex_unlock(>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(>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 = >kobj;
p->kobj.ktype = _part;
kobject_init(>kobj);
-   kobject_add(>kobj);
+   if (kobject_add(>kobj)) 
+   goto out_put;
if (!disk->part_uevent_suppress)
kobject_uevent(>kobj, KOBJ_ADD);
-   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
+   if (sysfs_create_link(>kobj, _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(>kobj, );
+   if (sysfs_create_file(>kobj, )) {
+   sysfs_remove_link (>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(>kobj, KOBJ_REMOVE);
+   kobject_del(>kobj);
+out_put:
+   kobject_put(>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

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
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 = >kobj;
>   p->kobj.ktype = _part;
>   kobject_init(>kobj);
> - kobject_add(>kobj);
> + if (kobject_add(>kobj)) 

Hm, here the structure needs to be freed as well (via kobject_put()).

> + return -1;
>   if (!disk->part_uevent_suppress)
>   kobject_uevent(>kobj, KOBJ_ADD);
> - sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
> + if(sysfs_create_link(>kobj, _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(>kobj, );
> + if (sysfs_create_file(>kobj, )) {
> + sysfs_remove_link (>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(>kobj, KOBJ_REMOVE);
> + kobject_del(>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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 <[EMAIL PROTECTED]> wrote:
> 
> 
> 
> > @@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, 
> > sector_t start, sector_t len,
> > p->kobj.parent = >kobj;
> > p->kobj.ktype = _part;
> > kobject_init(>kobj);
> > -   kobject_add(>kobj);
> > +   if (kobject_add(>kobj)) 
> > +   return -1;
> > if (!disk->part_uevent_suppress)
> > kobject_uevent(>kobj, KOBJ_ADD);
> > -   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
> > +   if(sysfs_create_link(>kobj, _subsys.kobj, "subsystem")) {
> > +   kobject_del(>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. 

>  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(>kobj, );
> > +   if (sysfs_create_file(>kobj, )) {
> > +   kobject_del(>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(>bd_mutex);
+   return -EBUSY;
+   }
mutex_unlock(>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(>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 = >kobj;
p->kobj.ktype = _part;
kobject_init(>kobj);
-   kobject_add(>kobj);
+   if (kobject_add(>kobj)) 
+   return -1;
if (!disk->part_uevent_suppress)
kobject_uevent(>kobj, KOBJ_ADD);
-   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
+   if(sysfs_create_link(>kobj, _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(>kobj, );
+   if (sysfs_create_file(>kobj, )) {
+   sysfs_remove_link (>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(>kobj, KOBJ_REMOVE);
+   kobject_del(>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",

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
On Mon, 29 Oct 2007 05:22:11 -0700,
Dirk Hohndel <[EMAIL PROTECTED]> wrote:



> @@ -390,20 +390,31 @@ void add_partition(struct gendisk *disk, int part, 
> sector_t start, sector_t len,
>   p->kobj.parent = >kobj;
>   p->kobj.ktype = _part;
>   kobject_init(>kobj);
> - kobject_add(>kobj);
> + if (kobject_add(>kobj)) 
> + return -1;
>   if (!disk->part_uevent_suppress)
>   kobject_uevent(>kobj, KOBJ_ADD);
> - sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
> + if(sysfs_create_link(>kobj, _subsys.kobj, "subsystem")) {
> + kobject_del(>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.



>   if (flags & ADDPART_FLAG_WHOLEDISK) {
>   static struct attribute addpartattr = {
>   .name = "whole_disk",
>   .mode = S_IRUSR | S_IRGRP | S_IROTH,
>   };
> 
> - sysfs_create_file(>kobj, );
> + if (sysfs_create_file(>kobj, )) {
> + kobject_del(>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)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 <[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(>bd_mutex);
+   return -EBUSY;
+   }
mutex_unlock(>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(>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 = >kobj;
p->kobj.ktype = _part;
kobject_init(>kobj);
-   kobject_add(>kobj);
+   if (kobject_add(>kobj)) 
+   return -1;
if (!disk->part_uevent_suppress)
kobject_uevent(>kobj, KOBJ_ADD);
-   sysfs_create_link(>kobj, _subsys.kobj, "subsystem");
+   if(sysfs_create_link(>kobj, _subsys.kobj, "subsystem")) {
+   kobject_del(>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(>kobj, );
+   if (sysfs_create_file(>kobj, )) {
+   kobject_del(>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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 [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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
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)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 [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)) {

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread 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 [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

Re: [PATCH] add_partition silently ignored errors

2007-10-29 Thread Cornelia Huck 
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/