* [-mm Patch]nbd: check the return value of sysfs_create_file
@ 2007-04-28 5:30 WANG Cong
2007-05-04 6:14 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: WANG Cong @ 2007-04-28 5:30 UTC (permalink / raw)
To: steve, pavel, Paul.Clements, akpm; +Cc: linux-kernel, Wang Ya-gang, Chen Li-jun
Since 'sysfs_create_file' is declared with attribute warn_unused_result, we must always check its return value carefully.
Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
---
--- linux-2.6.21-rc7-mm2/drivers/block/nbd.c.orig 2007-04-27 17:27:47.000000000 +0800
+++ linux-2.6.21-rc7-mm2/drivers/block/nbd.c 2007-04-27 17:47:32.000000000 +0800
@@ -373,7 +373,10 @@ static void nbd_do_it(struct nbd_device
BUG_ON(lo->magic != LO_MAGIC);
lo->pid = current->pid;
- sysfs_create_file(&lo->disk->kobj, &pid_attr.attr);
+ if (sysfs_create_file(&lo->disk->kobj, &pid_attr.attr)) {
+ printk(KERN_ERR "nbd: sysfs_create_file failed!");
+ return;
+ }
while ((req = nbd_read_stat(lo)) != NULL)
nbd_end_request(req);
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [-mm Patch]nbd: check the return value of sysfs_create_file 2007-04-28 5:30 [-mm Patch]nbd: check the return value of sysfs_create_file WANG Cong @ 2007-05-04 6:14 ` Andrew Morton 2007-05-04 7:39 ` WANG Cong 0 siblings, 1 reply; 3+ messages in thread From: Andrew Morton @ 2007-05-04 6:14 UTC (permalink / raw) To: WANG Cong Cc: steve, pavel, Paul.Clements, linux-kernel, Wang Ya-gang, Chen Li-jun On Sat, 28 Apr 2007 13:30:23 +0800 WANG Cong <xiyou.wangcong@gmail.com> wrote: > Since 'sysfs_create_file' is declared with attribute warn_unused_result, we must always check its return value carefully. > Well that's not really the reason for your patch. warn_unused_result is there to tell us that there are deeper problems in the code which need addressing: the failure to check the sysfs_create_file() return value means that bugs in the kernel can remain undetected, or can be harder to find. > > --- > > --- linux-2.6.21-rc7-mm2/drivers/block/nbd.c.orig 2007-04-27 17:27:47.000000000 +0800 > +++ linux-2.6.21-rc7-mm2/drivers/block/nbd.c 2007-04-27 17:47:32.000000000 +0800 > @@ -373,7 +373,10 @@ static void nbd_do_it(struct nbd_device > BUG_ON(lo->magic != LO_MAGIC); > > lo->pid = current->pid; > - sysfs_create_file(&lo->disk->kobj, &pid_attr.attr); > + if (sysfs_create_file(&lo->disk->kobj, &pid_attr.attr)) { > + printk(KERN_ERR "nbd: sysfs_create_file failed!"); > + return; > + } > > while ((req = nbd_read_stat(lo)) != NULL) > nbd_end_request(req); It would better saner to propagate this error back through callers: --- a/drivers/block/nbd.c~nbd-check-the-return-value-of-sysfs_create_file-fix +++ a/drivers/block/nbd.c @@ -366,23 +366,25 @@ static struct disk_attribute pid_attr = .show = pid_show, }; -static void nbd_do_it(struct nbd_device *lo) +static int nbd_do_it(struct nbd_device *lo) { struct request *req; + int ret; BUG_ON(lo->magic != LO_MAGIC); lo->pid = current->pid; - if (sysfs_create_file(&lo->disk->kobj, &pid_attr.attr)) { + ret = sysfs_create_file(&lo->disk->kobj, &pid_attr.attr); + if (ret) { printk(KERN_ERR "nbd: sysfs_create_file failed!"); - return; + return ret; } while ((req = nbd_read_stat(lo)) != NULL) nbd_end_request(req); sysfs_remove_file(&lo->disk->kobj, &pid_attr.attr); - return; + return 0; } static void nbd_clear_que(struct nbd_device *lo) @@ -572,7 +574,9 @@ static int nbd_ioctl(struct inode *inode case NBD_DO_IT: if (!lo->file) return -EINVAL; - nbd_do_it(lo); + error = nbd_do_it(lo); + if (error) + return error; /* on return tidy up in case we have a signal */ /* Forcibly shutdown the socket causing all listeners * to error _ ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [-mm Patch]nbd: check the return value of sysfs_create_file 2007-05-04 6:14 ` Andrew Morton @ 2007-05-04 7:39 ` WANG Cong 0 siblings, 0 replies; 3+ messages in thread From: WANG Cong @ 2007-05-04 7:39 UTC (permalink / raw) To: Andrew Morton Cc: steve, pavel, Paul.Clements, linux-kernel, Wang Ya-gang, Chen Li-jun On Thu, May 03, 2007 at 11:14:50PM -0700, Andrew Morton wrote: >On Sat, 28 Apr 2007 13:30:23 +0800 WANG Cong <xiyou.wangcong@gmail.com> wrote: > >> Since 'sysfs_create_file' is declared with attribute warn_unused_result, we must always check its return value carefully. >> > >Well that's not really the reason for your patch. > >warn_unused_result is there to tell us that there are deeper problems in >the code which need addressing: the failure to check the >sysfs_create_file() return value means that bugs in the kernel can remain >undetected, or can be harder to find. Oh, thanks very much for your pointing. > >> >> --- >> >> --- linux-2.6.21-rc7-mm2/drivers/block/nbd.c.orig 2007-04-27 17:27:47.000000000 +0800 >> +++ linux-2.6.21-rc7-mm2/drivers/block/nbd.c 2007-04-27 17:47:32.000000000 +0800 >> @@ -373,7 +373,10 @@ static void nbd_do_it(struct nbd_device >> BUG_ON(lo->magic != LO_MAGIC); >> >> lo->pid = current->pid; >> - sysfs_create_file(&lo->disk->kobj, &pid_attr.attr); >> + if (sysfs_create_file(&lo->disk->kobj, &pid_attr.attr)) { >> + printk(KERN_ERR "nbd: sysfs_create_file failed!"); >> + return; >> + } >> >> while ((req = nbd_read_stat(lo)) != NULL) >> nbd_end_request(req); > >It would better saner to propagate this error back through callers: > >--- a/drivers/block/nbd.c~nbd-check-the-return-value-of-sysfs_create_file-fix >+++ a/drivers/block/nbd.c >@@ -366,23 +366,25 @@ static struct disk_attribute pid_attr = > .show = pid_show, > }; > >-static void nbd_do_it(struct nbd_device *lo) >+static int nbd_do_it(struct nbd_device *lo) > { > struct request *req; >+ int ret; > > BUG_ON(lo->magic != LO_MAGIC); > > lo->pid = current->pid; >- if (sysfs_create_file(&lo->disk->kobj, &pid_attr.attr)) { >+ ret = sysfs_create_file(&lo->disk->kobj, &pid_attr.attr); >+ if (ret) { > printk(KERN_ERR "nbd: sysfs_create_file failed!"); >- return; >+ return ret; > } > > while ((req = nbd_read_stat(lo)) != NULL) > nbd_end_request(req); > > sysfs_remove_file(&lo->disk->kobj, &pid_attr.attr); >- return; >+ return 0; > } > > static void nbd_clear_que(struct nbd_device *lo) >@@ -572,7 +574,9 @@ static int nbd_ioctl(struct inode *inode > case NBD_DO_IT: > if (!lo->file) > return -EINVAL; >- nbd_do_it(lo); >+ error = nbd_do_it(lo); >+ if (error) >+ return error; > /* on return tidy up in case we have a signal */ > /* Forcibly shutdown the socket causing all listeners > * to error >_ Well, better code. ;) I didn't consider changing the type of nbd_do_it(). Thanks again! ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-05-04 7:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-04-28 5:30 [-mm Patch]nbd: check the return value of sysfs_create_file WANG Cong 2007-05-04 6:14 ` Andrew Morton 2007-05-04 7:39 ` WANG Cong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox