Re: [-mm3 PATCH] (Retry) Check the return value of kobject_add and etc.

[WANG Cong <xiyou.wangcong@gmail.com>]

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!