lkml.org 
[lkml]   [2007]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.
On Tue, Apr 10, 2007 at 03:18:15PM -0700, Andrew Morton wrote:
>On Tue, 10 Apr 2007 22:08:29 +0800
>WANG Cong <xiyou.wangcong@gmail.com> wrote:
>
>> Since kobject_add, sysfs_create_link and sysfs_create_file are marked as '__must_check', we must always check their return values.
>>

<snip>

>
>Your mail client replaces tabs with spaces - please fix it.
>

Oh, I did't notice that. Thank you. I will fix it soon.

>The code duplication is unpleasing. We normally do this as below (please
>review this code).
>

Er, I see.

>Note that I changed it to not send the KOBJ_REMOVE if we didn't send a
>KOBJ_ADD.
>

I think you are right.

>
>From: WANG Cong <xiyou.wangcong@gmail.com>
>
>Since kobject_add, sysfs_create_link and sysfs_create_file are marked as
>'__must_check', we must always check their return values.
>
>Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>---
>
> fs/partitions/check.c | 25 ++++++++++++++++++++-----
> 1 files changed, 20 insertions(+), 5 deletions(-)
>
>diff -puN fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc fs/partitions/check.c
>--- a/fs/partitions/check.c~partitions-check-the-return-value-of-kobject_add-etc
>+++ a/fs/partitions/check.c
>@@ -383,26 +383,41 @@ void add_partition(struct gendisk *disk,
> p->policy = disk->policy;
>
> if (isdigit(disk->kobj.name[strlen(disk->kobj.name)-1]))
>- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%sp%d",disk->kobj.name,part);
>+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%sp%d",
>+ disk->kobj.name, part);
^^^
Andrew, it seems that you left an additional whitespace in the above line (marked as ^^^).

> else
>- snprintf(p->kobj.name,KOBJ_NAME_LEN,"%s%d",disk->kobj.name,part);
>+ snprintf(p->kobj.name, KOBJ_NAME_LEN, "%s%d",
>+ disk->kobj.name, part);
^^^
Also here. ;-p

> 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.kset.kobj, "subsystem");
>+ if (sysfs_create_link(&p->kobj, &block_subsys.kset.kobj, "subsystem"))
>+ goto out_uevent;
> 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))
>+ goto out_link;
> }
> partition_sysfs_add_subdir(p);
> disk->part[part-1] = p;
>+ return;
>+
>+out_link:
>+ sysfs_remove_link(&p->kobj, "subsystem");
>+out_uevent:
>+ if (!disk->part_uevent_suppress)
>+ kobject_uevent(&p->kobj, KOBJ_REMOVE);
>+ kobject_del(&p->kobj);
>+out_put:
>+ kobject_put(&p->kobj);
> }
>
> static char *make_block_name(struct gendisk *disk)
>_


Your code is better. Thanks again!


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

\
 
 \ /
  Last update: 2007-04-11 08:45    [W:3.838 / U:0.548 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site