Messages in this thread | | | Date | Wed, 11 Apr 2007 14:45:25 +0800 | From | WANG Cong <> | Subject | Re: [-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/
| |