On Tue, Apr 10, 2007 at 03:18:15PM -0700, Andrew Morton wrote:
>On Tue, 10 Apr 2007 22:08:29 +0800
>WANG Cong <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
>
>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 <[EMAIL PROTECTED]>
>Acked-by: Cornelia Huck <[EMAIL PROTECTED]>
>Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
>---
>
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
