[-mm Patch]nbd: check the return value of sysfs_create_file

[-mm Patch]nbd: check the return value of sysfs_create_file

[WANG Cong]

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);

Re: [-mm Patch]nbd: check the return value of sysfs_create_file

[Andrew Morton]

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
_

Re: [-mm Patch]nbd: check the return value of sysfs_create_file

[WANG Cong]

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!