nbd: use list_for_each_entry_safe to make it more consolidated and readable

PATCH In review Denis Cheng 2 participants 7 messages Jul 19, 2007 – Jul 20, 2007

Subsystems (4)

Networking Network drivers Block layer Network block device (NBD)

lore

Share & tools

lore

Denis Cheng

author

2007-07-19 09:34 UTC #0 lore raw

PATCH 1/2 nbd: use list_for_each_entry_safe to make it more consolidated and readable

Thus the traverse of the loop may delete nodes, use the safe version.

Signed-off-by Denis Cheng <[email protected]>

---
 drivers/block/nbd.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

drivers/block/nbd.c +2 -4

index c129510..86639c0 100644
@@ -237,8 +237,7 @@ error_out:
 
 static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
 {
-	struct request *req;
-	struct list_head *tmp;
+	struct request *req, *n;
 	struct request *xreq;
 	int err;

@@ -249,8 +248,7 @@ static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
 		goto out;
 
 	spin_lock(&lo->queue_lock);
-	list_for_each(tmp, &lo->queue_head) {
-		req = list_entry(tmp, struct request, queuelist);
+	list_for_each_entry_safe(req, n, &lo->queue_head, queuelist) {
 		if (req != xreq)
 			continue;
 		list_del_init(&req->queuelist);

Signature · 1 line

1.5.2.2

Denis Cheng

author

2007-07-19 09:35 UTC #1 lore raw

PATCH 2/2 nbd: change a parameter's type to remove a memcpy call

this memcpy looks so strange, in fact it's merely a pointer dereference, so I change the parameter's type to refer it more directly, this could make the memcpy not needed anymore.

in the function nbd_read_stat where nbd_find_request is only once called, the parameter served should be transformed accordingly.

Signed-off-by Denis Cheng <[email protected]>

---
 drivers/block/nbd.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

drivers/block/nbd.c +2 -5

index 86639c0..a4d8508 100644
@@ -235,14 +235,11 @@ error_out:
 	return 1;
 }
 
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request *xreq)
 {
 	struct request *req, *n;
-	struct request *xreq;
 	int err;
 
-	memcpy(&xreq, handle, sizeof(xreq));
-
 	err = wait_event_interruptible(lo->active_wq, lo->active_req != xreq);
 	if (unlikely(err))
 		goto out;

@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
 		goto harderror;
 	}
 
-	req = nbd_find_request(lo, reply.handle);
+	req = nbd_find_request(lo, *(struct request **)reply.handle);
 	if (unlikely(IS_ERR(req))) {
 		result = PTR_ERR(req);
 		if (result != -ENOENT)

Signature · 1 line

1.5.2.2

Paul Clements

2007-07-19 17:35 UTC #2 lore raw

Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call

Quoted 6 lines · Denis Cheng

this memcpy looks so strange, in fact it's merely a pointer dereference, so I change the parameter's type to refer it more directly, this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called, the parameter served should be transformed accordingly.

This is really a matter of preference. The generated code ends up being about the same, I think, while your patch makes the call to nbd_find_request kind of obtuse. Also, the memcpy's are balanced between send_req and find_request, so you can quickly see how the data is being transferred (from req into handle, and then back again). Your patch makes this less clear, at least to me.

--
Paul

Quoted 34 lines

Signed-off-by Denis Cheng <[email protected]>

---
 drivers/block/nbd.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

drivers/block/nbd.c +2 -5

index 86639c0..a4d8508 100644
@@ -235,14 +235,11 @@ error_out:
 	return 1;
 }
 
-static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
+static struct request *nbd_find_request(struct nbd_device *lo, struct request *xreq)
 {
 	struct request *req, *n;
-	struct request *xreq;
 	int err;
 
-	memcpy(&xreq, handle, sizeof(xreq));
-
 	err = wait_event_interruptible(lo->active_wq, lo->active_req != xreq);
 	if (unlikely(err))
 		goto out;

@@ -297,7 +294,7 @@ static struct request *nbd_read_stat(struct nbd_device *lo)
 		goto harderror;
 	}
 
-	req = nbd_find_request(lo, reply.handle);
+	req = nbd_find_request(lo, *(struct request **)reply.handle);
 	if (unlikely(IS_ERR(req))) {
 		result = PTR_ERR(req);
 		if (result != -ENOENT)

rae l

author

2007-07-20 01:46 UTC #3 lore raw

Re: [PATCH 2/2] nbd: change a parameter's type to remove a memcpy call

Quoted 8 lines · Paul Clements

Quoted 6 lines · Denis Cheng
this memcpy looks so strange, in fact it's merely a pointer dereference, so I change the parameter's type to refer it more directly, this could make the memcpy not needed anymore.
in the function nbd_read_stat where nbd_find_request is only once called, the parameter served should be transformed accordingly.

Quoted 6 lines

This is really a matter of preference. The generated code ends up being about the same, I think, while your patch makes the call to nbd_find_request kind of obtuse. Also, the memcpy's are balanced between send_req and find_request, so you can quickly see how the data is being transferred (from req into handle, and then back again). Your patch makes this less clear, at least to me.

With one explicit memcpy stripped out, I think it's more clear to nbd_find_request.
In nbd_read_stat, the cast to (struct request **) is not apparent, I must admit; but I think the best solution is declaring other few structs to make it clear, it's due to the lack of description of nbd client and server communication protocol.

BTW, I think the nbd driver needs a clear documentation (its main site
http://nbd.sourceforge.net/ does not give it):

1. When nbd_find_request is needed to call, the 8 byte memory of char handle[8] field in struct nbd_reply actually stores a pointer (struct request *), that pointer is received from the network. Since a pointer is only meaningful to the host, transfering it over the network will be unreliable, I don't think it's a good design,

Signature · 5 lines

Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.

Paul Clements

2007-07-19 15:12 UTC #4 lore raw

In reply to Denis Cheng

Quoted 22 lines · Denis Cheng

Thus the traverse of the loop may delete nodes, use the safe version.

Signed-off-by Denis Cheng <[email protected]>

---
 drivers/block/nbd.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

drivers/block/nbd.c +1 -2

index c129510..86639c0 100644
@@ -237,8 +237,7 @@ error_out:
 
 static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
 {
-	struct request *req;
-	struct list_head *tmp;
+	struct request *req, *n;
 	struct request *xreq;
 	int err;

drivers/block/nbd.c static struct request *nbd_find_request(struct nbd_device *lo, char *handle) · line 254

Quoted 9 lines

drivers/block/nbd.c +1 -2

@@ -249,8 +248,7 @@ static struct request *nbd_find_request(struct nbd_device *lo, char *handle)
 		goto out;
 
 	spin_lock(&lo->queue_lock);
-	list_for_each(tmp, &lo->queue_head) {
-		req = list_entry(tmp, struct request, queuelist);
+	list_for_each_entry_safe(req, n, &lo->queue_head, queuelist) {
 		if (req != xreq)
 			continue;
 		list_del_init(&req->queuelist);

Could you name "n" as "tmp" (as in the previous code) so that it's clear that's only a temporary variable. Other than that, this looks good.

Thanks,
Paul

rae l

author

2007-07-19 15:55 UTC #5 lore raw

Quoted 2 lines · Paul Clements

Could you name "n" as "tmp" (as in the previous code) so that it's clear that's only a temporary variable. Other than that, this looks good.

Sure. I just use the name "n" as in the declaration of list_for_each_entry_safe in the header file <linux/list.h> I'll resend it a little later.

Quoted 4 lines

Thanks,
Paul

Signature · 5 lines

Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.

rae l

author

2007-07-19 16:48 UTC #6 lore raw

Quoted 6 lines · rae l

Quoted 2 lines · Paul Clements
Could you name "n" as "tmp" (as in the previous code) so that it's clear that's only a temporary variable. Other than that, this looks good.
Sure. I just use the name "n" as in the declaration of list_for_each_entry_safe in the header file <linux/list.h> I'll resend it a little later.

Signed-off-by Denis Cheng <[email protected]>

---
drivers/block/nbd.c | 6 ++----

1 files changed, 2 insertions(+), 4 deletions(-)

drivers/block/nbd.c +0 -0

index c129510..86639c0 100644
@@ -237,8 +237,7 @@ error_out:

static struct request *nbd_find_request(struct nbd_device *lo, char *handle) {
- struct request *req;
- struct list_head *tmp;

+	struct request *req, *tmp;
	struct request *xreq;
	int err;

@@ -249,8 +248,7 @@ static struct request *nbd_find_request(struct nbd_device *lo, char *handle)

		goto out;

	spin_lock(&lo->queue_lock);
-	list_for_each(tmp, &lo->queue_head) {
-		req = list_entry(tmp, struct request, queuelist);
+	list_for_each_entry_safe(req, tmp, &lo->queue_head, queuelist) {
		if (req != xreq)
			continue;
		list_del_init(&req->queuelist);

Signature · 5 lines

Denis Cheng
Linux Application Developer

"One of my most productive days was throwing away 1000 lines of code."
- Ken Thompson.