Skip to contentSkip to conversation

nbd: check the return value of sysfs_create_file

PATCH In review WANG Cong 2 participants 3 messages Apr 28, 2007 – May 4, 2007

Subsystems (2)
lore
Share & tools
lore

WANG Cong

author
#0 lore raw

PATCH 1/1 nbd: check the return value of sysfs_create_file

Since 'sysfs_create_file' is declared with attribute warn_unused_result, we must always check its return value carefully.

---

drivers/block/nbd.c renamed from drivers/block/nbd.c.orig +4 -1
@@ -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);
#1 lore raw
Quoted 2 lines · On Sat, 28 Apr 2007 13:30:23 +0800 WANG Cong <[email protected]> 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.

Quoted 7 lines

---

drivers/block/nbd.c renamed from drivers/block/nbd.c.orig +0 -0
@@ -373,7 +373,10 @@ static void nbd_do_it(struct nbd_device 
 	BUG_ON(lo->magic != LO_MAGIC);

drivers/block/nbd.c static void nbd_do_it(struct nbd_device · line 382

Quoted 10 lines
drivers/block/nbd.c renamed from drivers/block/nbd.c.orig +4 -1
 
 	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:

drivers/block/nbd.c renamed from drivers/block/nbd.c~nbd-check-the-return-value-of-sysfs_create_file-fix +9 -5
@@ -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

_

author
#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 2 lines · On Sat, 28 Apr 2007 13:30:23 +0800 WANG Cong <[email protected]> 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.

Quoted 64 lines
Quoted 8 lines

---

drivers/block/nbd.c renamed from drivers/block/nbd.c.orig +0 -0
@@ -373,7 +373,10 @@ static void nbd_do_it(struct nbd_device 
 	BUG_ON(lo->magic != LO_MAGIC);

drivers/block/nbd.c static void nbd_do_it(struct nbd_device · line 382

Quoted 10 lines
drivers/block/nbd.c renamed from drivers/block/nbd.c.orig +4 -1
 
 	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:

drivers/block/nbd.c renamed from drivers/block/nbd.c~nbd-check-the-return-value-of-sysfs_create_file-fix +9 -5
@@ -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
Quoted 1 line

_

Well, better code. ;) I didn't consider changing the type of nbd_do_it(). Thanks again!